[tablet mode] Multiple tabs remain selected when Virtual (on-screen) keyboard is displayed

VERIFIED FIXED in Firefox 42

Status

()

Firefox
Theme
P3
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: cornel_ionce, Assigned: Gijs)

Tracking

({regression})

Trunk
Firefox 43
All
Windows 10
regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(e10s+, firefox42 verified, firefox43 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150809030213

Reproduced on Windows 10 64-bit with a Microsoft Surface Pro 2 device using latest 42.0a1 Nightly.

STR:
1. Open Nightly.
2. Open several new tabs.
3. Switch through the tabs.

Expected Results:
The tabs can be successfully changed. No UI issues occur.

Actual Results:
All the tabs remain highlighted.

Screenshot: http://i.imgur.com/Pzk1v20.png

Notes:
It only reproduces on devices with on-screen keyboard.
This issue is a recent regression.

Regression Range:
Last-good buildid: 20150806030205
First-bad buildid: 20150807030210

Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=07befc6f54e7&tochange=91de9c670800

Caused by: 
Bug 1007063 - Virtual (on-screen) keyboard does not appear in Windows 8 desktop mode and windows 10 tablet mode
OS: Unspecified → Windows 10
Hardware: Unspecified → All
Blocks: 1007063
Keywords: regression
Cornel, when this bug happens, does moving the window off screen and back on screen fix the rendering of the tabs?

Gijs, are you able to reproduce this bug? There isn't anything within the OSK code that is specific to tabs. My hunch is that this has to do with the DismissOnScreenKeyboard function which is called during onblur, and that maybe the PostMessage call is screwing up our layout invalidation code? I don't have a tablet at hand so I can't reproduce this. If you can help, could you try commenting out the code within WinIMEHandler::DismissOnScreenKeyboard to see if this hunch has any basis to it?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(cornel.ionce)
Unfortunately, moving the window off screen and back does not fix the tab rendering issue.
Flags: needinfo?(cornel.ionce)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Cornel, when this bug happens, does moving the window off screen and back on
> screen fix the rendering of the tabs?
> 
> Gijs, are you able to reproduce this bug? There isn't anything within the
> OSK code that is specific to tabs. My hunch is that this has to do with the
> DismissOnScreenKeyboard function which is called during onblur, and that
> maybe the PostMessage call is screwing up our layout invalidation code? I
> don't have a tablet at hand so I can't reproduce this. If you can help,
> could you try commenting out the code within
> WinIMEHandler::DismissOnScreenKeyboard to see if this hunch has any basis to
> it?

Sorry for the delay here. I commented out the PostMessage call, that didn't seem to help.

I also investigated the layout invalidation thing. It's weird because I noticed that reattaching my keyboard and hovering over things didn't seem to help, so I went and looked with the browser console and toolbox.

It seems like tabs end up getting the visuallyselected attribute and that then doesn't get cleared.

I suspect this doesn't just have to do with dismissing but also with bringing up the keyboard. This moves focus (I would argue it shouldn't, but I imagine it must do?) and presumably means that the tab misses some part of the event (maybe it gets (simulated (because touch)) mousedown, but not (simulated) mouseup, or something?)

Cornel didn't specify how he's switching through the tabs, but I assume he's using touch. For obvious reasons I can't switch with the physical keyboard attached to reproduce this (because we never bring up the touch keyboard, even in tablet mode). Using an external USB mouse didn't seem to make a difference though.

Finally, this seems to not happen if you disable e10s, so I think this has something to do with how tabswitching is async when using e10s and something ends up racing something else somehow... so maybe we're just exposing something else about tab switching... CC'ing Mike and Bill in case they have ideas from bug 1132072.

Going to keep poking a bit to see if I can get a more concrete handle on this.
This seems to also have to do with whether the URLbar is focused in one of the tabs. We seem to keep track of that (or perhaps we keep track of selection or something like this) and cause the URL bar to be focused in those tabs. So if you fat-finger once and hit the urlbar, then things break and they keep breaking when switching to that tab (with the dismiss thing commented out... I will revert that and see if that keeps being a thing)
Right. So this seems to be happening because a MozLayerTreeReady event triggers updateDisplay (eventually). This causes us to change focus, which cause a number of paint (MozAfterPaint) events to go through updateDisplay before the MozLayerTreeReady event finishes processing. The fact that we update visibleTab at the top of the if (this.visibleTab !== showTab) block and only update lastVisibleTab all the way at the bottom then causes things to go haywire when the next few events come through.

When the OSK stuff is turned off, we don't trigger extra paint events just by running the focus code.

Bill, any idea how best to fix this? I would suggest just updating the visuallyselected + lastVisibleTab fields before changing focus (and maybe put the existing lastVisibleTab update in an else block?). Would that work for you or is there some reason we can't do that?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(wmccloskey)
Thanks for looking into this!

(In reply to :Gijs Kruitbosch from comment #5)
> Bill, any idea how best to fix this? I would suggest just updating the
> visuallyselected + lastVisibleTab fields before changing focus (and maybe
> put the existing lastVisibleTab update in an else block?). Would that work
> for you or is there some reason we can't do that?

I really don't like the idea of reentrancy in this code. It's hard enough to reason about as it is. Could we run the MozAfterPaint stuff based on setTimeout, possibly only if we're already running in the tab switch code (this could be tracked with a flag)?
Flags: needinfo?(wmccloskey)
Created attachment 8647567 [details]
MozReview Request: Bug 1192720 - prevent re-entrancy in event handling in tabbrowser.xml's _switcher, r?billm

Bug 1192720 - prevent re-entrancy in event handling in tabbrowser.xml's _switcher, r?billm
Attachment #8647567 - Flags: review?(wmccloskey)
Bill, does this look right? I didn't touch the other preActions/postActions invokers (particularly the load/unload timeouts and the requestTab) because I don't know if they need the same treatment or not...
Assignee: nobody → gijskruitbosch+bugs
(this is enough to fix the bug in question on my machine, at least)

Updated

3 years ago
tracking-e10s: ? → +
Comment on attachment 8647567 [details]
MozReview Request: Bug 1192720 - prevent re-entrancy in event handling in tabbrowser.xml's _switcher, r?billm

https://reviewboard.mozilla.org/r/16009/#review14283

Ship It!
Attachment #8647567 - Flags: review?(wmccloskey) → review+
Priority: -- → P3
Summary: [Tablet] Multiple tabs remain selected when Virtual (on-screen) keyboard is displayed → [tablet mode] Multiple tabs remain selected when Virtual (on-screen) keyboard is displayed
(In reply to :Gijs Kruitbosch from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=798b34311779

This somehow didn't get test jobs. Fixed with mozci, assuming this is green I'll land later today.
Comment on attachment 8647567 [details]
MozReview Request: Bug 1192720 - prevent re-entrancy in event handling in tabbrowser.xml's _switcher, r?billm

Bug 1192720 - prevent re-entrancy in event handling in tabbrowser.xml's _switcher, r?billm
(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=798b34311779
> 
> This somehow didn't get test jobs. Fixed with mozci, assuming this is green
> I'll land later today.

There are tabswitcher assertions in there because of the delaying that we do. Updated the mozreview request to cope with this. Bill, does this look right?

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=c24efe6ae418
(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to :Gijs Kruitbosch from comment #13)
> > (In reply to :Gijs Kruitbosch from comment #12)
> > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=798b34311779
> > 
> > This somehow didn't get test jobs. Fixed with mozci, assuming this is green
> > I'll land later today.
> 
> There are tabswitcher assertions in there because of the delaying that we
> do. Updated the mozreview request to cope with this. Bill, does this look
> right?
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c24efe6ae418

This is essentially green, at least...
Comment on attachment 8647567 [details]
MozReview Request: Bug 1192720 - prevent re-entrancy in event handling in tabbrowser.xml's _switcher, r?billm

mozreview not doing the right thing...
Attachment #8647567 - Flags: review+ → review?(wmccloskey)
Comment on attachment 8647567 [details]
MozReview Request: Bug 1192720 - prevent re-entrancy in event handling in tabbrowser.xml's _switcher, r?billm

https://reviewboard.mozilla.org/r/16009/#review14733

Ship It!
Attachment #8647567 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/fe896cc2c627
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Please request uplift to 42 when ready.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8647567 [details]
MozReview Request: Bug 1192720 - prevent re-entrancy in event handling in tabbrowser.xml's _switcher, r?billm

Approval Request Comment
[Feature/regressing bug #]: virtual keyboard support
[User impact if declined]: broken tab selection when virtual keyboard is on, but possibly also in other cases
[Describe test coverage new/current, TreeHerder]: sadly, no.
[Risks and why]: small to medium risk because of the impact on primary UI - but then, the regression in functionality right now is also significant. It's relatively early in the aurora cycle, and considering that mistakes in the first version of the patch triggered test failures, I am reasonably confident that this is safe for aurora
[String/UUID change made/needed]: no.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8647567 - Flags: approval-mozilla-aurora?
Comment on attachment 8647567 [details]
MozReview Request: Bug 1192720 - prevent re-entrancy in event handling in tabbrowser.xml's _switcher, r?billm

OK, let's take it then.
Attachment #8647567 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox42: --- → affected
Confirming that this issue is indeed fix using:
- latest Nightly, build ID: 20150825030212
- latest Aurora, build ID: 20150825004008.
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
status-firefox43: fixed → verified
Flags: qe-verify+
QA Contact: cornel.ionce
You need to log in before you can comment on or make changes to this bug.