Bug 220639

> I like to submit a patch for fixing this just that I don't know yet how to open a Web Inspector on Web Inspector :) on Chrome that can be achieved by entering Command+Option+I twice but in Safari that won't work apparently. On macOS, run: defaults write NSGlobalDomain WebKitDebugDeveloperExtrasEnabled -bool YES Then you'll get the "Inspect Element" option in the context menu. > The solution is to apply unicode-bidi: isolate to the specific element. I'd rather we added dir=auto as an attribute, rather than manually setting unicode-bidi: isolate, FWIW. Regardless—thanks for filing bugs (and even more so patches!) for all these bidi issues!

Thank you very much! Okay now that I have a working Web Inspector development environment using your instruction I see there is one case that dir="auto"/<bdi>/unicode-bidi:isolate would be overkill and that is a string that itself ends with bidi natural character like, "سیب." If we use isolate it will turn it to ".سیب" but there is a solution to this and is to use either unicode-bidi:embed or just to use dir="ltr" which implies unicode-bidi:embed anyway so I like to suggest something like dir="ltr": --- a/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js +++ b/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js @@ -532,6 +532,7 @@ WI.ConsoleMessageView = class ConsoleMessageView extends WI.Object parameters[i] = this._createRemoteObjectIfNeeded(parameters[i]); let builderElement = element.appendChild(document.createElement("span")); + builderElement.setAttribute("dir", "ltr"); let shouldFormatWithStringSubstitution = parameters[0].type === "string" && this._message.type !== WI.ConsoleMessage.MessageType.Result; // Single object (e.g. console result or logging a non-string object). Or, adding unicode-bidi:embed with CSS: --- a/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css +++ b/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css @@ -309,6 +309,10 @@ display: inline-block; } +.console-message-content { + unicode-bidi: embed; +} + @media (prefers-color-scheme: dark) { .console-message .syntax-highlighted { background-color: unset; --- a/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js +++ b/Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js @@ -532,6 +532,7 @@ WI.ConsoleMessageView = class ConsoleMessageView extends WI.Object parameters[i] = this._createRemoteObjectIfNeeded(parameters[i]); let builderElement = element.appendChild(document.createElement("span")); + builderElement.classList.add("console-message-content"); let shouldFormatWithStringSubstitution = parameters[0].type === "string" && this._message.type !== WI.ConsoleMessage.MessageType.Result; // Single object (e.g. console result or logging a non-string object). Or do you prefer dir="auto", which is OK to me also. Thanks!

Created attachment 417762 [details] PATCH Here is a proposed fix that applies dir="auto" to the saved variable name so that bidi algorithm won't be confused when the actually saved string has RTL text.

Created attachment 417763 [details] PATCH Here is a proposed fix that applies dir="auto" to the saved variable name so that bidi algorithm won't be confused when the actually saved string has RTL text.

Comment on attachment 417763 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=417763&action=review r=me > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:400 > + // So that bidi algorithm won't be confused when a saved string has RTL text Nit: "so that the bidi" Nit: WebKit style is to use a trailing period.

Comment on attachment 417763 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=417763&action=review r=me > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:400 > + // So that bidi algorithm won't be confused when a saved string has RTL text Nit: "so that the bidi" Nit: WebKit style is to use a trailing period.

(Apologies for the double comment, not sure what happened!)

BJ, do we not want to apply this to .formatted-string? > Or do you prefer dir="auto", which is OK to me also. Thanks! We definitely don't want unicode-bidi: embed for anything, I'd think, given we don't want the larger context to influence it (or it to influence the larger context). We might even want to split .formatted-string up so that the quotation marks don't have this applied to them.

> BJ, do we not want to apply this to .formatted-string? It is OK to me just that in the following image, all the lines are from the same text "سیب." (means apple if you wonder) https://i.imgur.com/sAqbjNx.png 0. "سیب." 1. The current situation, very bad, the bidi algorithm needs a help and $2 is mixed with the whole thing 2. If dir=auto be applied to .formatted-string 3. Current patch, what I see is expected that dir=auto is applied to $2's element, is achievable also if I could apply unicode-bidi:embed here which is considered not nice but I believe makes sense, it isn't that dangerous, web used to love it before isolate existence and its semantic is also OK here as it is just like applying dir=ltr here which itself is a safe thing, still. However if you thinks 2 is OK I am also with it also, as it is definitely much better than 1 anyway. Thanks!

Created attachment 417913 [details] PATCH

Actually all we need here is to use unicode-bidi: isolate but not dir=auto which we makes the text to be represented incorrectly, so let me propose a patch based on that which I believe will be safest thing to do here.

Created attachment 417975 [details] PATCH

Created attachment 417979 [details] PATCH It turned out the same issue can happen if regexp contains RTL script so updated the patch to consider that case also, the following image shows the issue. https://i.imgur.com/hRAALOj.png The text on the picture: "سیب." /سیب./ On the left, Safari, what currently happens, and on the right, Web Inspector of minibrowser, what I believe is expected behavior here. Sorry for the unintended spam btw.

Oh, no, I finally sat down and thought about this without getting interrupted and I'm sorry for being wrong before! We definitely want the base direction to be ltr, and we want it to be isolated from the surrounding context, as otherwise things get very surprising in JS context. So we want unicode-bidi: isolate; direction: ltr; as the used values (i.e., dir=ltr), and direction: ltr is the initial value we indeed inherit so this current patch looks good to me. So: non-reviewer r+.

Isolation is definitely the correct thing to do here. I would say in general, if there's anywhere in your UI where you're presenting user-generated strings inline with other content, you want to isolate. I'm a little unsure why you don't want dir=auto here, though. If you're dealing with RTL strings, wouldn't you want them handled as RTL?

Comment on attachment 417979 [details] PATCH r=me

(In reply to fantasai from comment #15) > Isolation is definitely the correct thing to do here. I would say in > general, if there's anywhere in your UI where you're presenting > user-generated strings inline with other content, you want to isolate. > > I'm a little unsure why you don't want dir=auto here, though. If you're > dealing with RTL strings, wouldn't you want them handled as RTL? Well, how do we expect a JS string such as "\u0633\u06cc\u0628\u002e" to get rendered in the console? I would expect "سیب." rather than ".سیب" as JS is normally treated as a LTR language? Likewise, I'd expect "<span>\u0633\u06cc\u0628\u002e</span>" to render as "<span>سیب.</span>" (i.e., LTR, which admittedly dir=auto will compute to). Having the two strings put the bidi neutral character in a different place seems bad, as it becomes ambiguous as to what the value of the string is? Like, is there any case where we'd want to render a JS string as RTL? That seems like that goes against the natural direction of JS?

> I'm a little unsure why you don't want dir=auto here, though. If you're dealing with RTL strings, wouldn't you want them handled as RTL? No, I expect I see what I've entered on evaluation line just like other browser and what I've entered at the first place, actually regex shows the issue just better. https://imgur.com/a/7PWC0xA Something like > /سشسیشسی/g < g/سشسیشسی/ Happens if dir=auto is used here and this is why it would be unexpected.

<rdar://problem/73473434>

Committed r271726: <https://trac.webkit.org/changeset/271726> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417979 [details].