Bug 84543 - unicode-bidi:plaintext treats embedded element with unicode-bidi:plaintext or isolate incorrectly
Summary: unicode-bidi:plaintext treats embedded element with unicode-bidi:plaintext or...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Igor Trindade Oliveira
URL:
Keywords:
Depends on:
Blocks: 50910
  Show dependency treegraph
 
Reported: 2012-04-22 05:35 PDT by Aharon (Vladimir) Lanin
Modified: 2016-03-10 11:27 PST (History)
13 users (show)

See Also:


Attachments
Test case 1: block plaintext around inline isolate. (1.24 KB, text/html)
2012-04-22 05:41 PDT, Aharon (Vladimir) Lanin
no flags Details
Test case 2: block plaintext around inline plaintext. (1.11 KB, text/html)
2012-04-22 05:42 PDT, Aharon (Vladimir) Lanin
no flags Details
Test case 3: inline plaintext around inline isolate. (1.24 KB, text/html)
2012-04-22 05:43 PDT, Aharon (Vladimir) Lanin
no flags Details
Test case 4: inline plaintext around inline plaintext. (1.12 KB, text/html)
2012-04-22 05:44 PDT, Aharon (Vladimir) Lanin
no flags Details
Patch V1 (12.55 KB, patch)
2014-11-21 11:33 PST, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (12.54 KB, patch)
2014-11-21 11:52 PST, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (12.52 KB, patch)
2014-11-21 13:32 PST, Igor Trindade Oliveira
bfulgham: review-
igor.oliveira: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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?