Stop using negative margins on Firefox #21
Conversation
cfe1fac to
08443c4
Compare
nigelmegitt
left a comment
There was a problem hiding this comment.
We need to leave a comment about this change, and a link to the Firefox bugzilla ticket, so future us can understand why this change is here.
I think it would also be nice to minimise the impact by only using the override 0px value for marginLeft on ee when ipd === "rl" and marginRight on ee when ipd==="lr" which I think should work. What do you think @robertbryer , too complicated?
nigelmegitt
left a comment
There was a problem hiding this comment.
Thanks, that looks good aside from the blank line deletions. I'll also load this up locally and verify it fixes the issue in the expected way for me.
|
|
||
| // Start element | ||
| if (context.ipd === "lr") { | ||
|
|
There was a problem hiding this comment.
It's a bit of an idiomatic code style thing, but these blank lines are deliberate and endemic in the library, so we should probably keep them. I think I once got a pull request bounced back by @palemieux for breaking the coding style like this, and it looks like it would be good to contribute this workaround back to sandflow/imscjs if we can.
|
Verified that the fix works on Firefox, and is not applied on Chrome. |
|
Don't we normally check in the built version? |
|
Typically no, only CI should be publishing build artefacts; really it should be uploading to npm instead of git pushing the dist folder, but there was a lack of clarity on npm licences at the time we started this project. |
Duplicated from @robertbryer 's pull request bbc#21 .
https://bugzilla.mozilla.org/show_bug.cgi?id=1502610
This will cause the positioning to be slightly off and should be removed when the bug is resolved.