Consider merging https://chromium.googlesource.com/chromium/blink/+/ad5ac746dbad9e71667dc873afbc0900d89c4a28
Created attachment 211221 [details] Patch
Created attachment 211222 [details] Test results without source changed (but with new test applied)
Comment on attachment 211221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211221&action=review > Source/WebCore/css/html.css:1150 > +textarea[dir=auto] { > + unicode-bidi: -webkit-plaintext; > +} The change log entry should mention about this change. > Source/WebCore/html/HTMLElement.cpp:835 > + if (hasTagName(inputTag)) { Use isHTMLInputElement instead. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:396 > + && (current->isRenderInline() || current->isRenderBlock())) { Why do we need to check for this condition? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:400 > + if (current != root) > + current = 0; > + else > + current = next; This is a very mysterious code. It walks up the render tree to find the first inline or block render object with unicode-bidi: -webkit-isolate, and then if it's root, set current to the original value. If not, then it resets (effectively setting it to root's firstChild). According to https://code.google.com/p/chromium/issues/detail?id=277687, we should be looking for the first strongly LTR or RTL letter. Given that, I don't see how this code can be correct. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:409 > + if (!current) > + current = root->firstChild(); > + > + while (current) { I don't like the fact current & next are re-purposed in the second loop. We should probably split either part into a separate function, and give much more descriptive variable names.
Comment on attachment 211221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211221&action=review > Source/WebCore/ChangeLog:21 > + * css/html.css: > + (textarea[dir=auto]): > + * html/HTMLElement.cpp: > + (WebCore::HTMLElement::directionality): > + * rendering/RenderBlockLineLayout.cpp: > + (WebCore::firstRenderObjectForDirectionalityDetermination): > + (WebCore::determinePlaintextDirectionality): > + (WebCore::constructBidiRunsForSegment): > + (WebCore::RenderBlock::layoutRunsAndFloatsInRange): > + (WebCore::RenderBlock::determineStartPosition): Especially given that this is merge, I think you should really have per-change comments explaining the rationale.
I changed the test case from Chromium Patch to following JSFiddle: Test - https://jsfiddle.net/jr0bv9h1/show I think it is fixed via recent work around directionality and bid by (@rniwa) since Safari Technology Preview 151 is showing it similar to all other browsers (Chrome Canary 106 and Firefox Nightly 105), note position of !, which is of right side in STP 151 and others but on left side in Safari 15.6.1. rniwa@webkit.org - Can you confirm if this is needed anymore since it seems to be fixed in STP 151. Thanks!
I don't think it's my change but this is working now.
IFC (modern line layout) made this path redundant.
(In reply to zalan from comment #7) > IFC (modern line layout) made this path redundant. patch even.