Summary: | unicode-bidi:plaintext treats embedded element with unicode-bidi:plaintext or isolate incorrectly | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aharon (Vladimir) Lanin <aharon> | ||||||||||||||||
Component: | CSS | Assignee: | 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
Aharon (Vladimir) Lanin
2012-04-22 05:35:18 PDT
Created attachment 138266 [details]
Test case 1: block plaintext around inline isolate.
Created attachment 138267 [details]
Test case 2: block plaintext around inline plaintext.
Created attachment 138268 [details]
Test case 3: inline plaintext around inline isolate.
Created attachment 138269 [details]
Test case 4: inline plaintext around inline plaintext.
I hope to take a look at this next week. FYI, this has been fixed in Blink. See https://code.google.com/p/chromium/issues/detail?id=276642, but watch out for https://code.google.com/p/chromium/issues/detail?id=299879 and https://code.google.com/p/chromium/issues/detail?id=301500. Created attachment 242055 [details]
Patch V1
The patch is fixing just the test cases 1 and 2. The test cases 3 and 4 are a different bug. Created attachment 242060 [details]
Patch
Created attachment 242067 [details]
Patch
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 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 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?
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! Thanks for retrying the reproduction cases!!! :D (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) |