1103635 - [e10s] New <select> dropdown doesn't honor RTL direction at all when e10s is enabled

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[ebrahim]

Attachment: Visually explained

1. Open data:text/html,<center><select><optgroup label="LTR"><option>A)</option></optgroup></select><div dir="rtl"><select><optgroup label="RTL"><option>A)</option></optgroup></select>
2. Compare dropdowns of the two select elements.

Expected:
Dropdown of second <select> which is RTL should be flipped

Actual:
It is same first dropdown and is specially when neutral characters (like dots and bracket involved) it would be broken and not usable at all.

Have a look at the attachment also.

Comment 1

[Simon Montagu :smontagu]

This seems to be a very recent regression: I can reproduce with mozilla-inbound but not with mozilla-central. What build are you testing with?

Comment 2

[ebrahim]

I am using Nightly builds, sorry that I didn't specified that.

about:buildconfig
Build Machine

B-2008-IX-0157
Source

Built from https://hg.mozilla.org/mozilla-central/rev/8c02f3280d0c
Build platform
target
x86_64-pc-mingw32
Build tools
Compiler 	Version 	Compiler flags
cl 	1800 	-TC -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -FS -wd4244 -wd4267 -wd4819 -we4553
cl 	1800 	-TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -FS -wd4251 -wd4244 -wd4267 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR- -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1 -Oi -Oy-
Configure arguments

--target=x86_64-pc-mingw32 --host=x86_64-pc-mingw32 --enable-crashreporter --enable-release --enable-update-channel=nightly --enable-update-packaging --enable-jemalloc --with-google-api-keyfile=/c/builds/gapi.data --with-google-oauth-api-keyfile=/c/builds/google-oauth-api.key --enable-signmar --enable-profiling --enable-js-diagnostics

Comment 3

[(no longer active)]

Alice, any chance you could please find the regression range here?  Thanks!

Comment 4

[Alice0775 White]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> Alice, any chance you could please find the regression range here?  Thanks!

I cannot reproduce on latest Nightly.
https://hg.mozilla.org/mozilla-central/rev/17de0f463944
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0 ID:20141128114334

Comment 5

[ebrahim]

Still reproducible on Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:37.0) Gecko/20100101 Firefox/37.0 https://hg.mozilla.org/mozilla-central/rev/17de0f463944

Comment 6

[Alice0775 White]

This is a not recent regression.
The problem happens on e10s but not non-e10s. 
This problem also happens on Lunux as well as Windows if e10s is enabled.

Comment 7

[Chris Peterson [:cpeterson]]

M6 to block e10s Aurora uplift.

Comment 8

[George Wright (:gw280) (needinfo me!)]

Attachment: rtl-select.patch

This propagates the text direction through to the relevant menuitems but it doesn't work because of a bug in nsTextBoxFrame where it refuses to do bidi processing unless RTL characters are present. I will file a separate bug for that.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8593560 [details] [diff] [review]
rtl-select.patch

Review of attachment 8593560 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks George!

::: toolkit/modules/SelectContentHelper.jsm
@@ +88,4 @@
>  
>  function buildOptionListForChildren(node) {
>    let result = [];
> +  let popupView = node.ownerDocument.defaultView;

I have no idea why the containing window is called "defaultView", but generally we call this "win" or "window" - since that's clearer. Can you change popupView -> win?

@@ +107,4 @@
>        let info = {
>          tagName: child.tagName,
>          textContent: textContent,
> +        textDirection: popupView.getComputedStyle(child, null).getPropertyValue("direction"),

The second argument to getComputedStyle is optional - you don't need to pass it here. And I think you can just do:

win.getComputedStyle(child).direction

::: toolkit/modules/SelectParentHelper.jsm
@@ +77,4 @@
>    for (let option of options) {
>      let item = element.ownerDocument.createElement("menuitem");
>      item.setAttribute("label", option.textContent);
> +    item.style.direction = option.textDirection;

So we have to do this for every individual menuitem? We can't do it for the popup once?

If so, we should mention that in a comment here.

Comment 10

[George Wright (:gw280) (needinfo me!)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> So we have to do this for every individual menuitem? We can't do it for the
> popup once?
> 
> If so, we should mention that in a comment here.

Yes, because you can have an individual option element with the rtl style set; something like:

<select>
<optgroup label="RTL">
  <option style="direction: rtl;">A)</option>
  <option>B)</option>
</optgroup>
</select>

And A) should be rendered as "(A" (ie - RTL) and B) should stay as "B)".

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Ah, yeah, that makes sense. Thanks.

Comment 12

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d29724d782

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f4d29724d782