Bug 84543

Summary: unicode-bidi:plaintext treats embedded element with unicode-bidi:plaintext or isolate incorrectly
Product: WebKit Reporter: Aharon (Vladimir) Lanin <aharon>
Component: CSSAssignee: Igor Trindade Oliveira <igor.oliveira>
Status: NEW ---    
Severity: Normal CC: ahmad.saleem792, bfulgham, commit-queue, eric, esprehn+autocc, glenn, igor.oliveira, kondapallykalyan, leviw, mitz, mmaxfield, playmobil, rniwa, simon.fraser, webkit-bug-importer, xji
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 50910    
Attachments:
Description Flags
Test case 1: block plaintext around inline isolate.
none
Test case 2: block plaintext around inline plaintext.
none
Test case 3: inline plaintext around inline isolate.
none
Test case 4: inline plaintext around inline plaintext.
none
Patch V1
none
Patch
none
Patch bfulgham: review-

Description Aharon (Vladimir) Lanin 2012-04-22 05:35:18 PDT
The current definition of unicode-bidi:plaintext in CSS Writing Modes Level 3 states:

This value behaves as ‘isolate’ except that for the purposes of the Unicode bidirectional algorithm, the base directionality of each bidi paragraph immediately contained by the element is determined not by the element's computed ‘direction’ as usual, but by following the heuristic in rules P2 and P3 of the Unicode bidirectional algorithm. An element immediately contains a bidi paragraph if the element itself, but none of its descendants, both contains the entire bidi paragraph and is either a block container or a bidi-isolating inline.

Thus, if you have inline element A around inline element B, where A has unicode-bidi:plaintext and B has unicode-bidi:isolate, then:

1. The paragraphs in B must get their base direction from B's direction property as usual, not from their content as would be the case if B has unicode-bidi:plaintext - even though B is inside A and A does have unicode-bidi:plaintext. The "immediately contained" definition was recently added to clarify this very point.

2. B's content must not affect the direction of the paragraph in A inside which B occurs (because, according to the spec, "for the purpose of bidi resolution in its containing bidi paragraph (if any), [an element with unicode-bidi:isolate] is treated as if it were an Object Replacement Character (U+FFFC)"). The same is true if B also had unicode-bidi:plaintext (because unicode-bidi:plaintext "behaves as ‘isolate’").

Unfortunately, neither 1 nor 2 holds in WebKit. The first two attachments show this when A is a block element. (I used a <div> with unicode-bidi:-webkit-isolate style, but it could have been done with a <pre> out of the box.) The next two attachment show that things are even worse when A is an inline element: the content after B completely disappears. Very strange and worrisome.
Comment 1 Aharon (Vladimir) Lanin 2012-04-22 05:41:12 PDT
Created attachment 138266 [details]
Test case 1: block plaintext around inline isolate.
Comment 2 Aharon (Vladimir) Lanin 2012-04-22 05:42:00 PDT
Created attachment 138267 [details]
Test case 2: block plaintext around inline plaintext.
Comment 3 Aharon (Vladimir) Lanin 2012-04-22 05:43:25 PDT
Created attachment 138268 [details]
Test case 3: inline plaintext around inline isolate.
Comment 4 Aharon (Vladimir) Lanin 2012-04-22 05:44:15 PDT
Created attachment 138269 [details]
Test case 4: inline plaintext around inline plaintext.
Comment 5 Levi Weintraub 2012-04-22 10:36:54 PDT
I hope to take a look at this next week.
Comment 7 Igor Trindade Oliveira 2014-11-21 11:33:13 PST
Created attachment 242055 [details]
Patch V1
Comment 8 Igor Trindade Oliveira 2014-11-21 11:37:05 PST
The patch is fixing just the test cases 1 and 2. The test cases 3 and 4 are a different bug.
Comment 9 Igor Trindade Oliveira 2014-11-21 11:52:34 PST
Created attachment 242060 [details]
Patch
Comment 10 Igor Trindade Oliveira 2014-11-21 13:32:34 PST
Created attachment 242067 [details]
Patch
Comment 11 Myles C. Maxfield 2014-12-01 11:56:48 PST
Comment on attachment 242067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242067&action=review

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:58
> +        if (isIsolated(current->style().unicodeBidi())

Maybe we want to stay inside root here?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:62
> +            else

I'm not sure this would ever occur. If I'm reading RenderBlockLineLayout correctly, every time we call this function, the root element's unicodeBidi() is Plaintext.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:78
> +            next = downcast<RenderElement>(*current).firstChild();

How do you know that this is a RenderElement?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:81
> +            while (current && current != root) {

Don't we have a better way of iterating through elements in order? Some sort of iterator class?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:101
> +    InlineIterator iter(rootElement, firstRenderObjectForDirectionalityDetermination(rootElement, iterator.renderer()), iterator.offset());

Is this really right? You are specifying the RenderObject input to InlineIterator, but keeping the original offset. What if the first RenderObject is shorter than the original iterator's offset?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:104
> +    observer.setStatus(BidiStatus(rootElement->style().direction(), isOverride(rootElement->style().unicodeBidi())));

Using the root element here seems fishy to me. What if something between iter.renderer() and root has its direction specified?
Comment 12 Myles C. Maxfield 2014-12-04 13:22:00 PST
Comment on attachment 242067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242067&action=review

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:81
>> +            while (current && current != root) {
> 
> Don't we have a better way of iterating through elements in order? Some sort of iterator class?

RenderIterator
Comment 13 Brent Fulgham 2016-03-10 11:27:03 PST
Comment on attachment 242067 [details]
Patch

This patch can't be applied to current WebKit sources. Could you please address Myles' review comments, using RenderIterator, and upload a new patch?
Comment 14 Ahmad Saleem 2022-06-09 17:37:41 PDT
I am able to reproduce this partially in Safari 15.5 on macOS 12.4.

Test Case 1 -> Reproducible:

Safari 15.5 - Does not match both boxes
Chrome Canary 104 - Matches both boxes
Firefox Nightly 103 - Matches both boxes

Test Case 1 -> Reproducible:

Safari 15.5 - Does not match both boxes
Chrome Canary 104 - Matches both boxes
Firefox Nightly 103 - Matches both boxes

Test Case 3 -> Matching Chrome but different from Firefox:

Safari 15.5 - Matches both boxes (Although in two lines)
Chrome Canary 104 - Matches both boxes (same as Safari)
Firefox Nightly 103 - Matches both boxes (In single line)

Test Case 4 -> Matching Chrome but different from Firefox:

Safari 15.5 - Matches both boxes (Although in two lines)
Chrome Canary 104 - Matches both boxes (same as Safari)
Firefox Nightly 103 - Matches both boxes (In single line)

If I am testing it incorrectly, please retest accordingly. Thanks!
Comment 15 Myles C. Maxfield 2022-06-09 18:22:47 PDT
Thanks for retrying the reproduction cases!!! :D
Comment 16 Radar WebKit Bug Importer 2022-08-08 11:19:25 PDT
<rdar://problem/98330916>
Comment 17 Ahmad Saleem 2022-09-11 03:51:44 PDT
(In reply to Ahmad Saleem from comment #14)
> I am able to reproduce this partially in Safari 15.5 on macOS 12.4.
> 
> Test Case 1 -> Reproducible:
> 
> Safari 15.5 - Does not match both boxes
> Chrome Canary 104 - Matches both boxes
> Firefox Nightly 103 - Matches both boxes
> 
> Test Case 1 -> Reproducible:
> 
> Safari 15.5 - Does not match both boxes
> Chrome Canary 104 - Matches both boxes
> Firefox Nightly 103 - Matches both boxes
> 
> Test Case 3 -> Matching Chrome but different from Firefox:
> 
> Safari 15.5 - Matches both boxes (Although in two lines)
> Chrome Canary 104 - Matches both boxes (same as Safari)
> Firefox Nightly 103 - Matches both boxes (In single line)
> 
> Test Case 4 -> Matching Chrome but different from Firefox:
> 
> Safari 15.5 - Matches both boxes (Although in two lines)
> Chrome Canary 104 - Matches both boxes (same as Safari)
> Firefox Nightly 103 - Matches both boxes (In single line)
> 
> If I am testing it incorrectly, please retest accordingly. Thanks!

Update from Safari Technology Preview 153 (not checked compared to other browsers except where we were just matching one):

Test Case 1 -> No more reproducible.. :-)

STP 153 - Matches both boxes
Chrome Canary 104 - Matches both boxes
Firefox Nightly 103 - Matches both boxes

Test Case 1 -> No more reproducible.. :-)

STP 153 - Matches both boxes
Chrome Canary 104 - Matches both boxes
Firefox Nightly 103 - Matches both boxes

Test Case 3 -> Matching Chrome but different from Firefox: [No difference from last in STP 153]

Safari 15.5 - Matches both boxes (Although in two lines)
Chrome Canary 104 - Matches both boxes (same as Safari)
Firefox Nightly 103 - Matches both boxes (In single line)

Test Case 4 -> Matching Chrome but different from Firefox: [No difference from last in STP 153]

Safari 15.5 - Matches both boxes (Although in two lines)
Chrome Canary 104 - Matches both boxes (same as Safari)
Firefox Nightly 103 - Matches both boxes (In single line)