Closed Bug 1193518 Opened 4 years ago Closed 4 years ago

Resolving the direction of elements with unicode-bidi: plaintext can leave a WritingMode in an inconsistent state

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The WritingMode::SetDirectionFromBidiLevel method toggles the writing-mode object's BidiDir if necessary to match the level passed in, but fails to update the InlineDir to match.

Currently, we get away with this because the relevant code in nsBidiPresUtils only looks at IsBidiLTR(), and doesn't access the InlineDir. But this will change with the implementation of sideways-left, where LTR will run from bottom to top.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Blocks: 1193519
Attachment #8646582 - Attachment description: Ensure that when we update a WritingMode after resolving di we set the inline-dir bit appropriately as well as the inline-bidi bit → Ensure that when we update a WritingMode after resolving dir=auto we set the inline-dir bit appropriately as well as the inline-bidi bit
Attachment #8646582 - Flags: review?(dholbert)
I don't 100% understand what's going on here. smontagu might be able to review this more quickly/intelligently than I can, if he has spare cycles to review things at the moment...

(If not, I can get to it after I've churned through some other pending reviews, though I'll need to do a bit of reading to better understand these bits & bidi-levels first.)
:smontagu, if you happen to have time to look at this, please feel free to steal the review from :dholbert. :)
Flags: needinfo?(smontagu)
Comment on attachment 8646582 [details] [diff] [review]
Ensure that when we update a WritingMode after resolving dir=auto we set the inline-dir bit appropriately as well as the inline-bidi bit

Review of attachment 8646582 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/WritingModes.h
@@ +499,5 @@
>    //XXX change uint8_t to UBiDiLevel after bug 924851
>    void SetDirectionFromBidiLevel(uint8_t level)
>    {
> +    if (IS_LEVEL_RTL(level) == IsBidiLTR()) {
> +      mWritingMode ^= eBidiMask | eInlineFlowMask;

I'm having trouble understanding why we're making this particular equality comparison, and why we want to invert these particular bits if this check succeeds.

Things I noticed when trying to review just now:
 - This function only gets called for frames with "unicode-bidi: plaintext", per [1].
 - I think this style ("unicode-bidi: plaintext") means we're supposed to ignore the 'direction' property *and* ignore the parent's bidirectional state, per [2].
 - BUT, it looks like we're caring about both here -- we're checking 'level' (which is influenced by our parent's bidi state I think?) and we're also checking IsBidiLTR (which is determined by our 'direction' property). [3]

So, I don't really understand what's going on here.  Explanatory code-comments would probably help me here (& others who are similarly-confused when reading this in the future.)

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=bb91cf98a42b#1060
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/unicode-bidi
[3] http://mxr.mozilla.org/mozilla-central/source/layout/generic/WritingModes.h?rev=e6eed9f588de#489
Comment on attachment 8646582 [details] [diff] [review]
Ensure that when we update a WritingMode after resolving dir=auto we set the inline-dir bit appropriately as well as the inline-bidi bit

[Canceling review for now, per above comment. Feel free to tag me for review again after clarifying the background here a bit, either here or in code comments, and I'll take another look once I'm back from vacation.]
Flags: needinfo?(jfkthame)
Attachment #8646582 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] (vacation 8/27-9/13) from comment #4)
> Comment on attachment 8646582 [details] [diff] [review]
> Ensure that when we update a WritingMode after resolving dir=auto we set the
> inline-dir bit appropriately as well as the inline-bidi bit
> 
> Review of attachment 8646582 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/WritingModes.h
> @@ +499,5 @@
> >    //XXX change uint8_t to UBiDiLevel after bug 924851
> >    void SetDirectionFromBidiLevel(uint8_t level)
> >    {
> > +    if (IS_LEVEL_RTL(level) == IsBidiLTR()) {
> > +      mWritingMode ^= eBidiMask | eInlineFlowMask;
> 
> I'm having trouble understanding why we're making this particular equality
> comparison, 

The passed-in |level| determines whether we want this WritingMode to be bidi-LTR or bidi-RTL; and IsBidiLTR tells us which it currently is. If they match, there's nothing to do; but if they differ (i.e. if one of them is bidi-RTL and the other is bidi-LTR), then the passed-in direction (determined by the plaintext algorithm) is the opposite of our current direction....

> and why we want to invert these particular bits if this check
> succeeds.

....and in that case, we're reversing the inline direction, so we need to flip the bidi-direction bit (tells us whether we're RTL in the bidi sense) AND the inline-flow bit (tells us whether the inline flow direction is against physical coordinates, i.e. physical RTL or BTT) from the values that were computed from the writing-mode, text-orientation and direction properties.

The relationship between these two bits (whether they're the same or opposite) is independent of bidi direction; it depends on writing mode and text orientation. That was set up during WritingMode initialization. Then, when resolving plaintext, we either leave them untouched or flip them both, so that the proper relationship is maintained.

> 
> Things I noticed when trying to review just now:
>  - This function only gets called for frames with "unicode-bidi: plaintext",
> per [1].
>  - I think this style ("unicode-bidi: plaintext") means we're supposed to
> ignore the 'direction' property *and* ignore the parent's bidirectional
> state, per [2].

Right.

>  - BUT, it looks like we're caring about both here -- we're checking 'level'
> (which is influenced by our parent's bidi state I think?) and we're also
> checking IsBidiLTR (which is determined by our 'direction' property). [3]

No, the |level| parameter here is determined (by nsBidiPresUtils::GetFrameBaseLevel) from the content of the frame whose WritingMode we are updating.

So when we're dealing with unicode-bidi: plaintext, the only thing that determines the bidi directionality is the element's content. The inline flow direction depends on that in combination with the writing mode and text orientation properties.

Does the above help to clarify? If it makes sense, I'm happy to add comments in the code, too.
Flags: needinfo?(jfkthame)
Comment on attachment 8646582 [details] [diff] [review]
Ensure that when we update a WritingMode after resolving dir=auto we set the inline-dir bit appropriately as well as the inline-bidi bit

Re-flagging the same patch for r?, following comments above; I'm happy to add some of this commentary into the source, too, if you think that's useful.
Attachment #8646582 - Flags: review?(dholbert)
Comment on attachment 8646582 [details] [diff] [review]
Ensure that when we update a WritingMode after resolving dir=auto we set the inline-dir bit appropriately as well as the inline-bidi bit

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Does the above help to clarify? If it makes sense, I'm happy to add comments
> in the code, too.

Yup, thanks. (Combined with some background reading to remind myself how bidi works in general, using the high-level overview in http://www.w3.org/International/tutorials/svg-tiny-bidi/ )

RE clarifying code-comments, a few ideas:

>+++ b/layout/generic/WritingModes.h
>@@ -494,22 +494,18 @@ public:
>   // For unicode-bidi: plaintext, reset the direction of the writing mode from
>   // the bidi paragraph level of the content
> 
>   //XXX change uint8_t to UBiDiLevel after bug 924851
>   void SetDirectionFromBidiLevel(uint8_t level)
>   {

So firstly: it wasn't clear to me (until now) that this "For unicode-bidi..." comment here is actually *documentation for this method*. That was partly due to the blank line, and partly due to me being unfamiliar with bidi. :) And maybe partly due to the phrasing being not 

It'd help to
 (1) expand the comment a bit.
 (2) drop the blank space after the end of the comment (or bring it inside a contiguous /** */) to make it clearer that it's documentation for this method.
and while we're at it:
 (3) explain the implementation a little bit (hinting at why these bits in particular are the ones we're flipping)

Possible replacement:
  /**
   * This function performs fixup for elements with 'unicode-bidi: plaintext'.
   * For these elements, we're supposed to *ignore* the 'direction' property,
   * but we actually already reacted to 'direction' in the WritingMode
   * constructor (oops!).  So here, we see if that happened to end up producing
   * the correct result, according to the passed-in bidi-level. If it did,
   * there's nothing to do.  If it didn't (i.e. if the rtl-ness doesn't match),
   * then we correct the direction (flipping the same bits that get flipped in
   * the constructor's CSS "direction"-based chunk).
   *
   * XXX change uint8_t to UBiDiLevel after bug 924851
   */
Attachment #8646582 - Flags: review?(dholbert) → review+
> And maybe partly due to the phrasing being not

(Sorry, I left this sentence unfinished -- meant to complete that as: "And maybe partly due to the comment not quite being clear enough to make sense to someone only somewhat familiar with how bidi & unicode-bidi:plaintext works".  Anyway, hopefully my suggested-text can help with some thoughts on clarifying that.)
Flags: needinfo?(smontagu)
Thanks for this. I rewrote your suggested comment a bit, but kept essentially the same content; hope that helps future readers here!
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0caef59ced9518d3fb31d435365cfb968307f7a
Bug 1193518 - Ensure that when we update a WritingMode after resolving dir=auto, we set the inline-dir bit appropriately as well as the inline-bidi bit. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/b0caef59ced9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.