Closed Bug 1113206 Opened 9 years ago Closed 9 years ago

<select> elements are flaky in vertical layout

Categories

(Core :: Layout: Form Controls, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: smontagu, Assigned: jfkthame)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files, 2 obsolete files)

Attached file 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
Attached file Listbox test case
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".
Assignee: nobody → 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.
Blocks: 1112954
Depends on: 1119475
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.
With this on top of your main patch here, things seem to work pretty well.
Attachment #8590387 - Flags: feedback?(smontagu)
Attachment #8590384 - Attachment description: Make nsComboboxControlFrame and nsListControlFrame use inline/block size instead of width/height → Make nsComboboxControlFrame and nsListControlFrame use inline/block size instead of width/height [smontagu's patch, refreshed]
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.
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.
Attachment #8613488 - Flags: review?(smontagu)
Attachment #8590384 - Attachment is obsolete: true
Comment on attachment 8590387 [details] [diff] [review]
patch 2 - Additional fixup for nsComboboxControlFrame and nsListControlFrame

Folded this into attachment 8613488 [details] [diff] [review].
Attachment #8590387 - Flags: feedback?(smontagu)
Attachment #8590387 - Attachment is obsolete: true
Blocks: 1170129
And filed bug 1170129 as a followup about the problem with e10s mode.
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
Attachment #8613488 - Flags: review?(smontagu) → review+
Depends on: 1172450
https://hg.mozilla.org/mozilla-central/rev/c838abaf38a4
https://hg.mozilla.org/mozilla-central/rev/c53efb981810
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1175094
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.
Attachment #8643633 - Flags: review?(roc)
Assignee: smontagu → jfkthame
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/8a31c820b381e29e2db38a713e1c4bf421097976
changeset:  8a31c820b381e29e2db38a713e1c4bf421097976
user:       Jonathan Kew <jkew@mozilla.com>
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
There is a sample page.
http://www.mongolfont.com/test/firefox/select.html
It seams not working correctly.
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.
Depends on: CVE-2016-2822
(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.
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?
Siquinbilige, 

I see that you have joined already bug 1170129 . So, there is not much we can do now.
I am using Windows 7 Ultimate service pack 1.
It was worked when I have browser.tabs.remote.autostart and browser.tabs.remote.autostart.2 already set to false.
Thank you.
It was worked when I have browser.tabs.remote.autostart and browser.tabs.remote.autostart.2 set to false.
Thank you.
You need to log in before you can comment on or make changes to this bug.