1271532 - [e10s] combobox display text not updated when moving through items using up/down key

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect, P3)

Description

[Jessica Jong [:jessica] (inactive)]

From Bug #1265968 comment 28:

The behavior is different between e10s and non-e10s: when dropdown list is open, moving through the items using up/down key, in e10s, the combobox display text will not be updated, but in non-e10s, the combobox display text is updated. So if user dismiss the dropdown list using Escape key or by clicking somewhere else, the final displayed text on e10s and non-e10s are different.

On chrome browser, when dropdown list is open, the display text is updated when moving through items using up/down key, but it is not updated when moving through items using mouse.

I think we should have the same behavior on e10s and non-e10s.

Comment 1

[Mats Palmgren (inactive)]

FYI, the Escape key behavior in non-e10s is a bug: bug 307345.

On OSX, the up/down keys should highlight the new value but
NOT change it when the dropdown menu is shown (another bug
in non-e10s).  Pressing up/down when the dropdown menu is
closed should open it on OSX (bug 350135), rather than
directly changing the value on other platforms.
(Safari has the correct behavior on OSX)

Comment 2

[Jessica Jong [:jessica] (inactive)]

I used Chrome as my reference browser, Safari behaves differently on this.
So, I wonder who decides what should be the behavior in Firefox? Or should we just align the behavior on e10s with non-e10s?

We should consider the following scenarios:
- Up/down key when dropdown list is closed
- Up/down key when dropdown list is open, then pressing enter or esc or dismiss dialog by clicking on another part of the page
- Mouse over when dropdown list is open, then pressing enter or esc or dismiss dialog by clicking on another part of the page

Comment 3

[Jessica Jong [:jessica] (inactive)]

I can get back to this after having a consensus.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

By default I'd say we should follow our own non-e10s model also in e10s.
If we want to change behavior, then we should change it everywhere, both in non-e10s and e10s.

Comment 5

[Mats Palmgren (inactive)]

> I used Chrome as my reference browser

Please don't.  Form control behavior is quite bad in Chrome.

The goal for form control look and feel in Firefox has always been to be as close
as possible to the native controls on the platform.  Safari is a good reference
on OSX.  IE11 is a good reference on Windows (at least for behavior).  On Linux,
native GTK apps are the reference.

In regards to highlighting options on Up/Down when the menu is open: yes, e10s must
certainly do that.  But if you can fix this and still allow Escape to reset the value
then I don't think we should intentionally introduce bug 307345 in e10s just to be
compatible with non-e10s.  But I agree with Olli to the extent that if you can't
implement the correct platform behavior for some reason, matching the non-e10s buggy
behavior might be better than having different e10s buggy behavior.

(FYI, the behavior we want on OSX is to highlight the option *in the menu* but not
change the actual value until the user commits it using Return.  Up/Down with a
closed menu should open it - with the currently selected option vertically
centered over the control.)

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Mats Palmgren (:mats) from comment #5)
> > I used Chrome as my reference browser
> 
> Please don't.  Form control behavior is quite bad in Chrome.
> 
> The goal for form control look and feel in Firefox has always been to be as
> close
> as possible to the native controls on the platform.  Safari is a good
> reference
> on OSX.  IE11 is a good reference on Windows (at least for behavior).  On
> Linux,
> native GTK apps are the reference.

I see. Does that mean we would have different behavior on different platform?

> 
> In regards to highlighting options on Up/Down when the menu is open: yes,
> e10s must
> certainly do that.  But if you can fix this and still allow Escape to reset
> the value
> then I don't think we should intentionally introduce bug 307345 in e10s just
> to be
> compatible with non-e10s.  But I agree with Olli to the extent that if you
> can't
> implement the correct platform behavior for some reason, matching the
> non-e10s buggy
> behavior might be better than having different e10s buggy behavior.

IMHO, I think we should focus on matching e10s and non-e10s behavior in this bug. And if we're going to change any of the current behavior (both e10s and non-e10s), we'll file a separate bug for that, like bug 307345.

> 
> (FYI, the behavior we want on OSX is to highlight the option *in the menu*
> but not
> change the actual value until the user commits it using Return.  Up/Down
> with a
> closed menu should open it - with the currently selected option vertically
> centered over the control.)

OSX differs from what we have now in that, when the dropdown list is open, there is no display text area, it is hidden behind the dropdown list. Also, it has a tick next to the currently selected item, which does not move when user moves the up/down key, so it makes sense for OSX to reset to the originally selected item.
Anyway, maybe we should follow the discussion of Esc key in bug 307345.

Comment 7

[Mats Palmgren (inactive)]

> I see. Does that mean we would have different behavior on different platform?

Yes.

> OSX differs from what we have now in that, when the dropdown list is open,
> there is no display text area, it is hidden behind the dropdown list.
> Also, it has a tick next to the currently selected item, which does not move
> when user moves the up/down key [...]

Great! That sounds like exactly the behavior we want on OSX.
If you can preserve that, while also fixing this bug on other platforms,
that would be ideal.

BTW, I'm fixing Up/Down so that they open the menu on OSX in bug 1272012
(for non-e10s).

Comment 8

[Jessica Jong [:jessica] (inactive)]

So, I tried to listen on "keypress" events in SelectParentHelper, but I didn't get any notifications. From [1], it seems that the menupop key events are handled by default. So, I tried to hack menulist.xml keypress event (just for testing), but that part of code was never reached, or I am doing something wrong...


[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide/PopupKeys#Handling_Keys_Within_Menus

Comment 9

[Jessica Jong [:jessica] (inactive)]

Some summary about the current progress.

In e10s, the dropdown list is a xul menupopup. From [1], a key listener added to a <menupopup> will not receive any key events. I tried to add the key listener to the document or window (using menulist.menupopup.ownerDocument.defaultView), but no luck so far. I don't think we want to change current xul menupopup behavior, do we?

Another option is to use the 'DOMMenuItemActive', but this event gets fired whenever an menu item is highlighted, so I get this on mouseovers as well, which is not what we want cause non-e10s does not update display text on mouseovers.

Another thought is, what if we match non-e10s behavior to e10s? that is, we do not update display text until an item is actually selected by clicking it or using the return key. Then, we won't have the Esc key problem in bug 307345. Of course, we'll need to fix lots of the current tests.

Olli, do you have any thoughts or suggestions about this? Thanks.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Mike knows the e10s popup setup here better than I do. He might have some ideas.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Removing the "#ifdef" in [1] solves this issue.

However, the code led me to Bug 1149745, which already discussed about this and the conclusion was to update the text only on Windows, to emulate the system behavior. So I guess what we need to do is match non-e10s behavior to e10s?


[1] https://hg.mozilla.org/mozilla-central/file/594736cae3ad/layout/xul/nsMenuPopupFrame.cpp#l1856

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Could we add some attribute to the menupopup indicating which behavior should be taken, and the default behavior would be then OS-dependent?
keyboardupdate="default" (which would mean the same as having no attribute) and 
keyboardupdate="select" ... or perhaps some better names. And then the UI for <select>'s popup would use the non-default

Comment 13

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #12)
> Could we add some attribute to the menupopup indicating which behavior
> should be taken, and the default behavior would be then OS-dependent?
> keyboardupdate="default" (which would mean the same as having no attribute)
> and 
> keyboardupdate="select" ... or perhaps some better names. And then the UI
> for <select>'s popup would use the non-default

That would mean, for <select>, we'll update the display text on key up/down while the menu is open, right?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Right, we'd have the non-e10s behavior also in e10s. Isn't that was this bug is about.

Comment 15

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #14)
> Right, we'd have the non-e10s behavior also in e10s. Isn't that was this bug
> is about.

Yep. I just though since the decision made in bug 1149745 was to do it only on Windows, this bug may be invalid or dup. :)

But adding the 'keyboardupdate' attribute is good, it'll bring us flexibility no matter what behavior would be. Will provide a patch for this.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP

Patch works but breaks browser_selectpopup.js on OSX:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b05b9a52058&selectedJob=21171872

Seems that it produces a weird behavior on OSX where the tick is moved but the highlight disappears while navigating. Figuring out why...

Comment 17

[Neil Deakin]

I'm confused here. Why does this bug exist?

The correct platform behaviour is already present for e10s select dropdowns and was implemented in bug 1149745.

It's the non-e10s behaviour that is incorrect on OSX (and also on GTK).

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Why are we implementing some UI changes only to e10s?

If e10s behavior is what we actually want, then we need to change non-e10s behavior.

Comment 19

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

(In reply to Olli Pettay [:smaug] from comment #18) 
> If e10s behavior is what we actually want, then we need to change non-e10s
> behavior.

I actually think that makes more sense, at least on OS X. To be clear, what I'm suggesting is that we alter the non-e10s dropdown such that when the selection changes, we do not update the underling <select> until the popup has been closed. That's closer to the native behaviour of OS X, and it's what we're shipping with e10s. I also agree that we want a convergence between e10s and non-e10s behaviours where possible.

An examination of the work in bug 1149745 might be useful, in finding ways to disable its "fix" for the OS X and GTK cases.

Comment 20

[Jessica Jong [:jessica] (inactive)]

I think we are on the same page now from comment 17, 18 and 19, so closing this e10s bug.

Comment 21

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

(In reply to Jessica Jong [:jessica] from comment #20)
> I think we are on the same page now from comment 17, 18 and 19, so closing
> this e10s bug.

Hey jessica - do you know if a bug is on file to change the OS X and GTK behaviours so that non-e10s is made to match the e10s behaviour?

Comment 22

[Jessica Jong [:jessica] (inactive)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #21)
> (In reply to Jessica Jong [:jessica] from comment #20)
> > I think we are on the same page now from comment 17, 18 and 19, so closing
> > this e10s bug.
> 
> Hey jessica - do you know if a bug is on file to change the OS X and GTK
> behaviours so that non-e10s is made to match the e10s behaviour?

Just filed one, here it is - bug 1275493! :)