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)

55 Branch
defect
Not set
normal

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)

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)
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)
Attached file testcase
Attached image screenshot
Flags: needinfo?(jfkthame)
This bug affects users of WhatsApp Web.
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]
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.
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]
This is simply the reduced testcase above, in reftest form. Fails on current trunk code.
Attachment #8939335 - Flags: review?(dholbert)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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)
Attachment #8939335 - Flags: review?(dholbert) → review+
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?
Flags: needinfo?(jfkthame)
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)
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)
Attachment #8939336 - Attachment is obsolete: true
Attachment #8939336 - Flags: review?(dholbert)
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+
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]
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
https://hg.mozilla.org/mozilla-central/rev/ea9e9cf12f6b
https://hg.mozilla.org/mozilla-central/rev/a37695e59774
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Is this something we need to consider uplifting to Beta for Fx58 or can it ride the 59 train?
Flags: needinfo?(jfkthame)
Version: 59 Branch → 55 Branch
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)
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 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+
Hi Florin, can we have some QA's help to verify this?
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
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.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
You need to log in before you can comment on or make changes to this bug.