1113206 - <select> elements are flaky in vertical layout

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Simon Montagu :smontagu]

Attachment: Combobox test case

Set vertical writing mode in the attached test cases

Problems include:

###!!! ASSERTION: Computed overflow area must contain frame bounds: 'aNewSize.width == 0 || aNewSize.height == 0 || r->width == nscoord_MAX || r->height == nscoord_MAX || (mState & NS_FRAME_SVG_LAYOUT) || r->Contains(nsRect(nsPoint(0,0), aNewSize))', file /Users/smontagu/mozwork/hgtree/mozilla/layout/generic/nsFrame.cpp, line 7423

###!!! ASSERTION: We should not have a computed height here!: 'aReflowState.ComputedHeight() == nscoord(1 << 30)', file /Users/smontagu/mozwork/hgtree/mozilla/layout/forms/nsListControlFrame.cpp, line 476

The line block-size of the options is much too wide.
The combobox is completely misrendered.
The alignment of the list box is inconsistent between vertical-lr and vertical-rl

Some of these issues, not all, are fixed by patches which I will be attaching to bug 1079154 once they pass try

Comment 1

[Simon Montagu :smontagu]

Attachment: Listbox test case

Comment 2

[Simon Montagu :smontagu]

Attachment: w-i-p patch: use inline and block size in listcontrols and combobox controls instead of width and height

This fixes the assertions mentioned above, but reintroduces two new ones: 

ASSERTION: FinishReflowChild with unconstrained container width!: 'aContainerWidth != NS_UNCONSTRAINEDSIZE', file ... layout/generic/nsContainerFrame.cpp, line 961
ASSERTION: FinishReflowChild with unconstrained container width!: 'aContainerWidth != NS_UNCONSTRAINEDSIZE', file ... layout/generic/nsContainerFrame.cpp, line 1104

(in the first one "FinishReflowChild" is a typo or mis-copy-paste for "ReflowChild".

Comment 3

[Simon Montagu :smontagu]

The patch also seems to fix the testcase in bug 1112954. 

Bug 1119475 will make a lot of difference to the rendering here, and maybe the assertions as well. I'll revisit after that is checked in.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Make nsComboboxControlFrame and nsListControlFrame use inline/block size instead of width/height [smontagu's patch, refreshed]

I was about to work on something like this, when I remembered you'd got some WIP already. So instead I've just un-bitrotted this, and tested with current trunk code; looking pretty good. I have a couple of additional fixes that I'll add as a followup patch.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Additional fixup for nsComboboxControlFrame and nsListControlFrame

With this on top of your main patch here, things seem to work pretty well.

Comment 6

[Jonathan Kew [:jfkthame]]

The main remaining issue, AFAICT, is that the native-theme rendering of <select> elements looks terrible when we try to apply it in vertical mode, at least on OS X. That's probably best split off into a separate bug. For now, using -moz-appearance:none on vertical <select>s avoids the worst of the ugliness.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Make nsComboboxControlFrame and nsListControlFrame use logical coordinates and support vertical writing modes

Simon, this is your earlier patch here, rebased and with my followup folded in. I've marked this r=me, but I guess it'd be good for you to review my changes to it before we consider it ready to land. AFAICT, together with a widget patch (coming next), this makes things work pretty well in non-e10s windows. In e10s, we have a further problem because the dropdown for a <select> control gets proxied to the parent process, which then uses a XUL popup to render it, and that doesn't have vertical support. But I think we should make that a followup bug.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: patch 2: Disable use of Cocoa and Gtk native-theme dropdown rendering that doesn't play well with vertical writing mode

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8590387 [details] [diff] [review]
patch 2 - Additional fixup for nsComboboxControlFrame and nsListControlFrame

Folded this into attachment 8613488 [details] [diff] [review].

Comment 10

[Jonathan Kew [:jfkthame]]

And filed bug 1170129 as a followup about the problem with e10s mode.

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 8613488 [details] [diff] [review]
Make nsComboboxControlFrame and nsListControlFrame use logical coordinates and support vertical writing modes

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

r=me for your fixes

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c838abaf38a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c53efb981810

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c838abaf38a4
https://hg.mozilla.org/mozilla-central/rev/c53efb981810

Comment 14

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a603ab448b67

Comment 15

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ffe8f328931

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: followup - Add simple test for <select> elements in vertical writing mode

The patches here landed with only manual testing; here's a basic mochitest that checks the popup is appearing on the expected (increasing block-direction) side of the element. This won't work under e10s until bug 1170129 is fixed, so skipping it for now.

Comment 17

[Jonathan Kew [:jfkthame]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/8a31c820b381e29e2db38a713e1c4bf421097976
changeset:  8a31c820b381e29e2db38a713e1c4bf421097976
user:       Jonathan Kew <[e-mail]>
date:       Wed Aug 05 12:43:24 2015 +0100
description:
Bug 1113206 followup - Add simple test for <select> elements in vertical writing mode. r=roc

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a31c820b381

Comment 20

[siqinbilige]

Attachment: firefox_select_vertical.png

There is a sample page.
http://www.mongolfont.com/test/firefox/select.html
It seams not working correctly.

Comment 21

[Gérard Talbot]

Siquinbilige,

Type in the address bar: 
about:preferences#general 
and then uncheck Activate multi-processus mode . Click OK and then Firefox should restart with the same opened tabs. Then try again your own test.

Comment 22

[siqinbilige]

(In reply to Gérard Talbot from comment #21)
> Siquinbilige,
> 
> Type in the address bar: 
> about:preferences#general 
> and then uncheck Activate multi-processus mode . Click OK and then Firefox
> should restart with the same opened tabs. Then try again your own test.

I did not able to found setting item multi-processus on mode on 57.0.2.
How to set it to select working correctly in vertical writing mode.

Comment 23

[Gérard Talbot]

Siquinbilige,

Thank you for reporting this.

I am using Firefox 59.0a1 buildID=20171211105838 and the expanded parts of selects are no longer displayed according to their associated writing-mode in these tests (and others too)

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-form-controls-sideways-lr-Example-4.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-form-controls-sideways-rl-Example-4.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-form-controls-vertical-lr-Example-4.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-form-controls-vertical-rl-Example-4.xht

. And multi-process is supposed to be ON, to be enabled by default in 57.0.2 (and in 59.0a1). So, right now, I do not have an answer to your question.

I looked at the support files on/about multi-process and read

https://support.mozilla.org/en-US/questions/1170439

but I have browser.tabs.remote.autostart and browser.tabs.remote.autostart.2 already set to true.

Right now, I believe we should create a new bug report on this, after searching bug reports on multi-process (electrolysis) and select.

Siquinbilige, I am using Linux 4.9.0-4-amd64 Debian 9.3 (codename stretch). What is your operating system?

Comment 24

[Gérard Talbot]

Siquinbilige, 

I see that you have joined already bug 1170129 . So, there is not much we can do now.

Comment 25

[siqinbilige]

I am using Windows 7 Ultimate service pack 1.

Comment 26

[siqinbilige]

It was worked when I have browser.tabs.remote.autostart and browser.tabs.remote.autostart.2 already set to false.
Thank you.

Comment 27

[siqinbilige]

It was worked when I have browser.tabs.remote.autostart and browser.tabs.remote.autostart.2 set to false.
Thank you.