Closed
Bug 1426042
Opened 6 years ago
Closed 6 years ago
Bidi reordering failure with zero-length ::after pseudo-element [was: The text is displayed in the LTR direction]
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
VERIFIED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | wontfix |
firefox58 | --- | verified |
firefox59 | --- | verified |
People
(Reporter: over68, Assigned: jfkthame)
References
Details
(Keywords: regression)
Attachments
(5 files, 1 obsolete file)
908.59 KB,
text/html
|
Details | |
30.71 KB,
image/png
|
Details | |
1.20 KB,
text/html
|
Details | |
3.71 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.81 KB,
patch
|
dholbert
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Steps to reproduce: 1. Open https://onedrive.live.com/download?cid=F96BA52A2AF70D03&resid=F96BA52A2AF70D03%211486&authkey=AKpkh-HtpsjhClg. Actual results: The text is displayed in the LTR direction. Screenshot https://1drv.ms/i/s!AgMN9yoqpWv5i08ZVhHS-tbfjnwH Expected results: Should be displayed in the RTL direction. Note: I can also reproduce this bug in https://onedrive.live.com/download?cid=F96BA52A2AF70D03&resid=F96BA52A2AF70D03%211488&authkey=AJZA31lXqO27Wik.
Flags: needinfo?(jfkthame)
Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7ad815515bd315ac0da9ed9b8c32b1f5dfeb27bf&tochange=34eb6c45444316f0b9e0a86f0b7a22aa1d3ae86f Regressed by: bug 1370053
Blocks: 1370053
Keywords: regression
Assignee | ||
Comment 2•6 years ago
|
||
Please attach testcase and screenshot files to the bug, rather than posting links to a file-hosting service. The screenshot link from comment 0 tells me it is not available...
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 6•6 years ago
|
||
Yes, confirmed that this appears to be a regression from bug 1370053. Note that we do *not* have test coverage for the original issue in bug 1370053, so we'll need to check manually that any fix here doesn't re-break that case. The failure here occurs because we fail to reorder frames properly when there is a ::before or ::after pseudo-element that has empty content (i.e. content: "", not content: none).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jfkthame)
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: The text is displayed in the LTR direction → Bid reordering failure with zero-length ::before/::after pseudo-elements [was: The text is displayed in the LTR direction]
Assignee | ||
Comment 7•6 years ago
|
||
In this testcase, all three examples should render the same, but currently the middle one (with empty ::before and ::after) fails to reorder. The first example (reference) has explicit empty <span> children before and after the actual text, and works fine. The second example, based on the Whatsapp page, uses ::before and ::after to insert empty pseudo-elements which should give the same result as the explicit <span>s in the reference. The third example puts a zero-width space into each of the pseudo-elements; this is sufficient to make their frames re-order as expected.
Assignee | ||
Updated•6 years ago
|
Summary: Bid reordering failure with zero-length ::before/::after pseudo-elements [was: The text is displayed in the LTR direction] → Bid reordering failure with zero-length ::after pseudo-element [was: The text is displayed in the LTR direction]
Assignee | ||
Comment 8•6 years ago
|
||
This is simply the reduced testcase above, in reftest form. Fails on current trunk code.
Attachment #8939335 -
Flags: review?(dholbert)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•6 years ago
|
||
And this fixes the regression (and in local testing, I've confirmed it does not re-introduce the original issue from bug 1370053). Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c05bb908f0c6c13598a22f8395ed49563ba8290.
Attachment #8939336 -
Flags: review?(dholbert)
Updated•6 years ago
|
Attachment #8939335 -
Flags: review?(dholbert) → review+
Comment 10•6 years ago
|
||
Comment on attachment 8939336 [details] [diff] [review] Ensure we don't forget to set bidi data when required on a zero-length trailing frame Review of attachment 8939336 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsBidiPresUtils.cpp @@ +891,5 @@ > + // If we ended with a zero-length fragment, don't forget to store > + // its bidi data before breaking out of the loop (bug 1426042). > + if (isTextFrame && contentTextLength == 0) { > + frame->AdjustOffsetsForBidi(0, 0); > + storeBidiDataToFrame(); I admit I don't fully understand what the contextual code is doing, or what this change is precisely doing. Was the problem that the "break" at the end of this clause (not quoted here) makes us skip the AdjustOffsetsForBidi/storeBidiDataToFrame calls further down... https://searchfox.org/mozilla-central/rev/b24e6342d744c5a83fab5c15972e11eeb69d68e6/layout/base/nsBidiPresUtils.cpp#907-912 ...and we need those calls (and used to do them earlier pre-bug 1370053)? (It does look like bug 1370053 moved those calls to their current location further down, basically.) If I'm understanding correctly, I'm willing to accept that this is generally sane. But one question -- in the already-existing callsite (the searchfox link above), we call storeBidiDataToFrame() unconditionally, regardless of isTextFrame and contentTextLength. Should we be doing that here, too? Or if it's fine the way you've got it right now, maybe update the code-comment to make it clearer why the logic differs between these two callsites?
Updated•6 years ago
|
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 11•6 years ago
|
||
Fair question. I'll admit I don't fully understand all this either. :\ The patch was designed as a minimal fix for the specific regression observed, based on inspecting the frame tree and observing that in the empty-::after example we failed to record bidi data on the pseudo's frame, and then figuring out how the patch from bug 1370053 would have caused that. But on reflection I think you're right that it makes sense to do storeBidiDataToFrame() unconditionally (and probably to call AdjustOffsetsForBidi regardless of contentTextLength, too). I don't currently know of a testcase where this will make an observable difference (a non-text frame will always have contributed one character such as ZWSP or OBJECT-REPLACEMENT or LINE-SEPARATOR to the text buffer, so I guess we shouldn't run out of text at that point?), but it seems the more logical thing to do. I pushed a try run with an updated patch, and nothing significant seems to have changed/broken: https://treeherder.mozilla.org/#/jobs?repo=try&revision=553fb2f99e310714f115d4f5d7a5ae49e0546936&selectedJob=154087406.
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 12•6 years ago
|
||
As discussed above, I think this makes sense although I have not found any observable difference in behavior from the previous patch.
Attachment #8939836 -
Flags: review?(dholbert)
Assignee | ||
Updated•6 years ago
|
Attachment #8939336 -
Attachment is obsolete: true
Attachment #8939336 -
Flags: review?(dholbert)
Comment 13•6 years ago
|
||
Comment on attachment 8939836 [details] [diff] [review] Ensure we don't forget to set bidi data when required on a zero-length trailing frame Review of attachment 8939836 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! This seems fine to me.
Attachment #8939836 -
Flags: review?(dholbert) → review+
Updated•6 years ago
|
Summary: Bid reordering failure with zero-length ::after pseudo-element [was: The text is displayed in the LTR direction] → Bidi reordering failure with zero-length ::after pseudo-element [was: The text is displayed in the LTR direction]
Comment 14•6 years ago
|
||
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea9e9cf12f6b Reftest for bidi reordering where line has an empty trailing ::after pseudo-element. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/a37695e59774 Ensure we don't forget to set bidi data when required on a zero-length trailing frame. r=dholbert
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ea9e9cf12f6b https://hg.mozilla.org/mozilla-central/rev/a37695e59774
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 16•6 years ago
|
||
Is this something we need to consider uplifting to Beta for Fx58 or can it ride the 59 train?
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(jfkthame)
Version: 59 Branch → 55 Branch
Assignee | ||
Comment 17•6 years ago
|
||
Given that this is a user-visible regression affecting real sites for bidi users, and the patch is nice and simple, I think it'd be worth uplifting so as to fix the issue sooner.
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 18•6 years ago
|
||
Comment on attachment 8939836 [details] [diff] [review] Ensure we don't forget to set bidi data when required on a zero-length trailing frame Approval Request Comment [Feature/Bug causing the regression]: bug 1370053 [User impact if declined]: incorrect layout/text ordering in certain bidi edge-cases [Is this code covered by automated tests?]: yes, reftest in tree [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no, but manual testing of bug 1370053 to confirm it remains fixed would be good [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: minor change to ensure we don't skip storing bidi data on the frame in an edge case, inadvertently broken in bug 1370053 [String changes made/needed]: none
Attachment #8939836 -
Flags: approval-mozilla-beta?
Comment 19•6 years ago
|
||
Comment on attachment 8939836 [details] [diff] [review] Ensure we don't forget to set bidi data when required on a zero-length trailing frame Fix a bidi rendering regression. Beta58+.
Attachment #8939836 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•6 years ago
|
||
Hi Florin, can we have some QA's help to verify this?
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Comment 21•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b5147bc4e384 https://hg.mozilla.org/releases/mozilla-beta/rev/5b67ac2c216b
Flags: in-testsuite+
Comment 22•6 years ago
|
||
I have reproduced this issue using Firefox 59.0a1 (20171218100313)on Windows 8.1 x64 using a RTL build. I can confirm this issue is fixed, I verified using Firefox 59.0a1 (2018.01.10)on Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.13.2. Tomorrow when appear the 58.0b16 build I will verify there too.
Comment 23•6 years ago
|
||
I can confirm this issue is fixed, I verified using Firefox 58.0b16 using an AR build(RTL)on Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.
Updated•6 years ago
|
Flags: needinfo?(florin.mezei)
You need to log in
before you can comment on or make changes to this bug.
Description
•