RESOLVED DUPLICATE of bug 284620 65617
Make unicode-bidi:isolate the default for block elements instead of unicode-bidi:embed 
https://bugs.webkit.org/show_bug.cgi?id=65617
Summary Make unicode-bidi:isolate the default for block elements instead of unicode-b...
Aharon (Vladimir) Lanin
Reported 2011-08-03 06:33:08 PDT
When a block element is set to display:inline, it is desirable for it to behave bidi-wise as it would if it were display:inline-block. Under CSS2.1, there was no way to accomplish this, and unicode-bidi:embed was used for lack of a better choice. Now that we have unicode-bidi:isolate, it should be the default instead, as the HTML5 spec (http://dev.w3.org/html5/spec/Overview.html#non-replaced-elements) says it should be.
Attachments
test case (591 bytes, text/html)
2011-08-03 06:57 PDT, Aharon (Vladimir) Lanin 		no flags
Details
test case (664 bytes, text/html)
2011-08-03 07:08 PDT, Aharon (Vladimir) Lanin 		no flags
Details
Patch (2.22 KB, patch)
2020-06-09 07:06 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (30.96 KB, patch)
2020-06-10 02:08 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Safari 15.5 matches other browsers (361.87 KB, image/png)
2022-06-15 14:56 PDT, Ahmad Saleem 		no flags
Details
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Aharon (Vladimir) Lanin
Comment 1 2011-08-03 06:57:07 PDT
Created attachment 102778 [details] test case
Aharon (Vladimir) Lanin
Comment 2 2011-08-03 07:08:04 PDT
Created attachment 102780 [details] test case
Eric Seidel (no email)
Comment 3 2011-08-08 09:24:20 PDT
This is going to make the bidi-isolate code even more performance-critical than it already is. :( Oh well.
Aharon (Vladimir) Lanin
Comment 4 2011-08-09 04:50:02 PDT
Please note that unicode-bidi:isolate should not have any effect (and thus can be completely ignored) on elements that already constitute separate bidi paragraphs, e.g. elements with display other than inline, or that have float, or that have absolute position, etc. The default is meant for the case when a "block" element like div gets assigned display:inline. This should be a fairly rare case.
Eric Seidel (no email)
Comment 5 2011-09-29 11:45:42 PDT
I suspect this may be as simple as adding the CSS from the HTML5 spec into html.css in WebCore: http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#bidirectional-text http://trac.webkit.org/browser/trunk/Source/WebCore/css/html.css The important piece is of course the LayoutTests which get landed with any change.
Aharon (Vladimir) Lanin
Comment 6 2011-11-17 02:13:55 PST
The change needs to be effective for "block" elements whether or not they have a dir attribute. Rules like div {display:block; unicode-bidi:isolate;} are not sufficient because the generic [dir] {unicode-bidi:embed;} is considered more specific and overrides them for elements with a dir attribute. See http://www.w3.org/Bugs/Public/show_bug.cgi?id=14850.
Aharon (Vladimir) Lanin
Comment 7 2013-09-24 08:28:42 PDT
*** Bug 74988 has been marked as a duplicate of this bug. ***
Aharon (Vladimir) Lanin
Comment 8 2020-05-26 06:53:47 PDT
Please note that this change was made in Chrome and Mozilla years ago.
Noam Rosenthal
Comment 9 2020-05-26 06:54:51 PDT
(In reply to Aharon (Vladimir) Lanin from comment #8) > Please note that this change was made in Chrome and Mozilla years ago. It was in Mozilla... In Chrome I've just revived an old patch that was never merged. https://chromium-review.googlesource.com/c/chromium/src/+/1081302
Aharon (Vladimir) Lanin
Comment 10 2020-05-26 08:10:28 PDT
Sorry, I got confused between https://bugs.chromium.org/p/chromium/issues/detail?id=296863 and https://bugs.chromium.org/p/chromium/issues/detail?id=391260. The latter, which is much more important than the former, was indeed fixed in 2016. Note, however, that the corresponding WebKit bug, https://bugs.webkit.org/show_bug.cgi?id=134630, is still NEW. That's the more important one.
Noam Rosenthal
Comment 11 2020-05-26 08:11:36 PDT
(In reply to Aharon (Vladimir) Lanin from comment #10) > Sorry, I got confused between > https://bugs.chromium.org/p/chromium/issues/detail?id=296863 and > https://bugs.chromium.org/p/chromium/issues/detail?id=391260. The latter, > which is much more important than the former, was indeed fixed in 2016. > > Note, however, that the corresponding WebKit bug, > https://bugs.webkit.org/show_bug.cgi?id=134630, is still NEW. That's the > more important one. Thanks! Good to know.
Noam Rosenthal
Comment 12 2020-06-01 07:46:52 PDT
New discussion in WhatWG: https://github.com/whatwg/html/issues/5594
Noam Rosenthal
Comment 13 2020-06-02 00:42:11 PDT
(In reply to Noam Rosenthal from comment #11) > (In reply to Aharon (Vladimir) Lanin from comment #10) > > Sorry, I got confused between > > https://bugs.chromium.org/p/chromium/issues/detail?id=296863 and > > https://bugs.chromium.org/p/chromium/issues/detail?id=391260. The latter, > > which is much more important than the former, was indeed fixed in 2016. > > > > Note, however, that the corresponding WebKit bug, > > https://bugs.webkit.org/show_bug.cgi?id=134630, is still NEW. That's the > > more important one. > https://bugs.webkit.org/show_bug.cgi?id=134630 has been resolved. Waiting for consensus on https://github.com/whatwg/html/issues/5594 before tackling this one.
Noam Rosenthal
Comment 14 2020-06-09 07:06:01 PDT
Created attachment 401438 [details] Patch
Noam Rosenthal
Comment 15 2020-06-09 07:06:57 PDT
(In reply to Noam Rosenthal from comment #14) > Created attachment 401438 [details] > Patch Uploaded a patch so that EWS can find all kinds of platform-specific failing tests.
Noam Rosenthal
Comment 16 2020-06-10 02:08:12 PDT
Created attachment 401523 [details] Patch
EWS Watchlist
Comment 17 2020-06-10 02:08:56 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Darin Adler
Comment 18 2020-06-10 11:11:00 PDT
Comment on attachment 401523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401523&action=review > Source/WebCore/css/html.css:1279 > -bdi, output { > +address, blockquote, center, div, figure, figcaption, footer, form, header, hr, > +legend, listing, main, p, plaintext, pre, summary, xmp, article, aside, h1, h2, > +h3, h4, h5, h6, hgroup, nav, section, table, caption, colgroup, col, thead, > +tbody, tfoot, tr, td, th, dir, dd, dl, dt, menu, ol, ul, li, bdi, output { > unicode-bidi: isolate; > } Why not put "unicode-bidi: isolate" right after each "display: block", since the rule is that they go together? If we did that we could see that it’s done correctly. When done this way, to verify it’s correct we have to carefully scan this list to notice that nothing extra is included and nothing is left out, which seems impractical.
Noam Rosenthal
Comment 19 2020-06-11 01:37:28 PDT
(In reply to Darin Adler from comment #18) > Comment on attachment 401523 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401523&action=review > > > Source/WebCore/css/html.css:1279 > > -bdi, output { > > +address, blockquote, center, div, figure, figcaption, footer, form, header, hr, > > +legend, listing, main, p, plaintext, pre, summary, xmp, article, aside, h1, h2, > > +h3, h4, h5, h6, hgroup, nav, section, table, caption, colgroup, col, thead, > > +tbody, tfoot, tr, td, th, dir, dd, dl, dt, menu, ol, ul, li, bdi, output { > > unicode-bidi: isolate; > > } > > Why not put "unicode-bidi: isolate" right after each "display: block", since > the rule is that they go together? If we did that we could see that it’s > done correctly. When done this way, to verify it’s correct we have to > carefully scan this list to notice that nothing extra is included and > nothing is left out, which seems impractical. It's copied directly from https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering, I thought it would be easier to keep them in sync-ish and track later if we keep them similar.
Ebrahim Byagowi
Comment 20 2021-01-20 10:18:31 PST
Bump, this really worths the hassle and great that this amount of work is done.
Ahmad Saleem
Comment 21 2022-06-15 14:56:51 PDT
Created attachment 460261 [details] Safari 15.5 matches other browsers I am unable to reproduce this bug in Safari 15.5 on macOS 12.4 by using attached test case and both lines look identical as required by expected result but I am not clear from the code perspective whether it is done correctly or not but from the output, it is rendering same across all browsers as shown in the attached screenshot. Thanks!
Ebrahim Byagowi
Comment 22 2022-06-17 08:49:39 PDT
(In reply to Ahmad Saleem from comment #21) > Created attachment 460261 [details] > Safari 15.5 matches other browsers > > I am unable to reproduce this bug in Safari 15.5 on macOS 12.4 by using > attached test case and both lines look identical as required by expected > result but I am not clear from the code perspective whether it is done > correctly or not but from the output, it is rendering same across all > browsers as shown in the attached screenshot. Thanks! Code-wise, basically this https://searchfox.org/mozilla-central/source/layout/style/res/html.css#39-92 should be copied to https://searchfox.org/mozilla-central/source/layout/style/res/html.css#39-92 and layout tests WebKit has in the repository should be updated. A test case, data:text/html;charset=utf8,شی<div style="display:%20inline">(ظطز) should get Firefox's result (not even Chrome result)
Radar WebKit Bug Importer
Comment 23 2022-07-12 15:04:52 PDT
<rdar://problem/96912726>
Brent Fulgham
Comment 24 2022-07-12 15:05:15 PDT
We match Chrome, but apparently Firefox is right but Chrome is wrong? @Myles: Can you confirm what we should do here?
Myles C. Maxfield
Comment 25 2022-07-16 19:59:03 PDT
The test case from 2011 was fixed by https://bugs.webkit.org/show_bug.cgi?id=134630. This bug is about the test case Ebrahim described in https://bugs.webkit.org/show_bug.cgi?id=65617#c22.
Darin Adler
Comment 26 2024-01-22 16:12:26 PST
*** Bug 267887 has been marked as a duplicate of this bug. ***
Darin Adler
Comment 27 2024-01-22 16:51:38 PST
For the record, I’m OK with copying the rule from the specification, despite my suggestion that maintenance would be easier if these were paired with "display: block". So that comment should not block this. Since Myles isn’t working on the project any more, who can move this forward?
Ahmad Saleem
Comment 28 2024-01-22 16:52:51 PST
(In reply to Darin Adler from comment #27) > For the record, I’m OK with copying the rule from the specification, despite > my suggestion that maintenance would be easier if these were paired with > "display: block". So that comment should not block this. Since Myles isn’t > working on the project any more, who can move this forward? Happy to give it a try. I will try to do changes from Noam and give credit and will do draft PR and then fix all remaining test cases (as best as I can). :-)
Ahmad Saleem
Comment 29 2024-02-08 18:04:56 PST
Just to update - I have this draft PR ready will all tests rebased but now we have WPT tests, so I will sync and then rebase my PR and then it would be ready for review (need to update - commit log as well for PR): Draft PR - https://github.com/WebKit/WebKit/pull/23084
Ahmad Saleem
Comment 30 2024-12-14 13:03:43 PST
With 287842@main, we are now matching and have fixed failing test case mentioned in comment 22. I am not on system, so will mark it dupe to other bug later. Additionally, I carved out my PR to make it easier to merge and not do everything in one go.
Daniel Holbert
Comment 31 2024-12-14 13:24:38 PST
My bug 267887 has a link to another self-describing testcase that'd perhaps be worth double-checking in patched builds: https://bug1874033.bmoattachments.org/attachment.cgi?id=9375895 Firefox and Chrome show: 'isolate' (as expected) epiphany/gnome-web v46 (based on WebKit) shows: 'normal' UNEXPECTED ...but I assume patched WebKit will match Firefox/Chrome.
Ahmad Saleem
Comment 32 2024-12-14 15:09:08 PST
(In reply to Daniel Holbert from comment #31) > My bug 267887 has a link to another self-describing testcase that'd perhaps > be worth double-checking in patched builds: > > https://bug1874033.bmoattachments.org/attachment.cgi?id=9375895 > > Firefox and Chrome show: > 'isolate' (as expected) > > epiphany/gnome-web v46 (based on WebKit) shows: > 'normal' UNEXPECTED > > ...but I assume patched WebKit will match Firefox/Chrome. Can confirm, it also worked on WebKit ToT.
Ahmad Saleem
Comment 33 2024-12-14 15:09:29 PST
(In reply to Ahmad Saleem from comment #32) > (In reply to Daniel Holbert from comment #31) > > My bug 267887 has a link to another self-describing testcase that'd perhaps > > be worth double-checking in patched builds: > > > > https://bug1874033.bmoattachments.org/attachment.cgi?id=9375895 > > > > Firefox and Chrome show: > > 'isolate' (as expected) > > > > epiphany/gnome-web v46 (based on WebKit) shows: > > 'normal' UNEXPECTED > > > > ...but I assume patched WebKit will match Firefox/Chrome. > > Can confirm, it also worked on WebKit ToT. On this <div>, the value of 'unicode-bidi' is: 'isolate' (as expected)
Ahmad Saleem
Comment 34 2024-12-14 15:10:08 PST
*** This bug has been marked as a duplicate of bug 284620 ***
Note You need to log in before you can comment on or make changes to this bug.