Closed Bug 1313403 Opened 8 years ago Closed 8 years ago

[e10s] Keyboard cursor is active in background tab instead of newly opened active tab

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Version:
52 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox49 --- unaffected
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + verified

People

(Reporter: Virtual, Assigned: Gijs)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

(Keywords: multiprocess, nightly-community, regression)

Attachments

(1 file)

Bug 1313403 - text (focus) goes to the wrong browser after foreground tab change,
(deleted), text/x-review-board-request
mak
: review+
florian
: review+
Details 
STR:
1. Have only one blank New Tab without anything in it
2. Open this URL - https://translate.google.com/?hl=en#auto/en/
3. Wait about 1-2s
4. Open again that URL - https://translate.google.com/?hl=en#auto/en/
5. Start writing something and see that all written text isn't shown in active second tab, but instead it's shown in first tab.
Has Regression Range: --- → yes
Has STR: --- → yes
Summary: [e10s] Keyboard cursor is active in background tab instead of active tab → [e10s] Keyboard cursor is active in background tab instead of newly opened active tab
[Tracking Requested - why for this release]: Regression

On stable builds the regression started in Firefox 48, where e10s is by default enabled.
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox-esr45: --- → unaffected
tracking-firefox49: --- → ?
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Track 51+ as regression.

Hi :jimm,
Can you help shed some light here or help find an owner for this bug?
tracking-firefox51: ? → +
Flags: needinfo?(jmathies)
(In reply to Virtual_ManPL [:Virtual] - (ni? me) from comment #0)
> STR:
> 1. Have only one blank New Tab without anything in it
> 2. Open this URL - https://translate.google.com/?hl=en#auto/en/
> 3. Wait about 1-2s
> 4. Open again that URL - https://translate.google.com/?hl=en#auto/en/
> 5. Start writing something and see that all written text isn't shown in
> active second tab, but instead it's shown in first tab.

When you say to open a URL, do you mean open it in the foreground (about:blank) tab, or a different tab?

FWIW, I'm not able to reproduce in Nightly using current steps. I'm also not sure what product/component this to go to. It doesn't appear to be plugin related. Maybe toolkit/focus manager?

We should get reliable str first.
Flags: needinfo?(jmathies)
Keywords: qawanted
Keywords: steps-wanted
Flags: needinfo?(virtual)
(In reply to Jim Mathies [:jimm] from comment #3)
> When you say to open a URL, do you mean open it in the foreground
> (about:blank) tab, or a different tab?
> 
> FWIW, I'm not able to reproduce in Nightly using current steps.
> [...]
> We should get reliable str first.

More detailed STR (as old one could be misleading and not fully complete):
1. Open https://translate.google.com/?hl=en#auto/en/
2. Bookmark it in Bookmarks Toolbar
3. Open new window with Ctrl+N keyboard shortcut
4. Open newly added bookmark from step #1 in foreground active tab
5. Wait about 2 seconds
6. Create next new foreground active tab with this URL using with Ctrl keyboard shortcut or "Open in New Tab" menu option
7. Start writing something and see that all written text isn't shown in newly opened foreground active second tab, but instead it's shown in background first tab.
Flags: needinfo?(virtual)
Keywords: qawanted, steps-wanted
Can you obtain a regression range for this behaviour with e10s, or was it always broken with e10s?

CC'ing Neil who might have more ideas about this.
Flags: needinfo?(virtual)
I can reproduce the problem on the URL but also on any web page
So, I think all page will be affected.

1. Bookmark data:text/html,<body>  or about:notexsit or this bug page
2. Open about:home in a foreground tab[A]
3. Open the bookmark in foreground new tab[B] by Middle click
4. Type something
5. Swithch to tab[A]

Actual Results:
All text are shown in tab[A]
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5083173624d5d3ef87034601a8acecbc83b3060e&tochange=adb484f84dec4b9d6a216ef3f4e1887fc3ae8084

Regressed by:  adb484f84dec	Gijs Kruitbosch — Bug 1000458 - stop races in location bar <return> handling code, r=mak


BTW, I cannot reproduce on 48.0.21 49.0.2, 50.0b11 and 51.0a2.
Blocks: 1000458
Flags: needinfo?(mak77)
Flags: needinfo?(gijskruitbosch+bugs)
Thank you Alice0775 White very much for finding very detailed regression range!

Today I was wondering what could cause this regression between last good build (2016-10-03) and first bad build (2016-10-04)
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=955840bfd3c20eb24dd5a01be27bdc55c489a285&tochange=42c95d88aaaa7c2eca1d278399421d437441ac4d
status-firefox49: affected → unaffected
status-firefox50: affected → unaffected
status-firefox51: affected → unaffected
tracking-firefox49: ? → ---
tracking-firefox50: ? → ---
tracking-firefox51: + → ---
Flags: needinfo?(virtual)
Keywords: regressionwindow-wanted
Version: 48 Branch → 52 Branch
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Flags: needinfo?(gijskruitbosch+bugs)
Shouldn't this be handled lower-level e.g. in the tabbrowser binding? Why did you choose to patch openLinkIn?
(In reply to Dão Gottwald [:dao] from comment #10)
> Shouldn't this be handled lower-level e.g. in the tabbrowser binding?

(If not lower than that. The cursor being stuck in a background browser feels like something front-end code shouldn't even be able to trigger.)
(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > Shouldn't this be handled lower-level e.g. in the tabbrowser binding?
> 
> (If not lower than that. The cursor being stuck in a background browser
> feels like something front-end code shouldn't even be able to trigger.)

I agree with the idea that we shouldn't be able to focus a background browser.

However, this is a direct regression from my manipulating this code in bug 1000458. There I basically replaced gBrowser.selectedBrowser with a passed-in current browser (which defaulted to selectedBrowser if absent). I didn't realize (ugh) at the time that the use of the selected browser for the focus call was kind of important, in the sense that focusing a different browser is nonsensical.

I could just revert that specific line to simply be gBrowser.selectedBrowser.focus(), but I actually think that's kind of bogus as well, in the sense that we shouldn't be bothering to focus the currently selected browser when you've opened a link in a new background tab or somesuch, which could be moving focus e.g. if you used bookmarks and the focus was in the url / search bar.

Anyway, the patch is focused specifically on addressing the regression, not on the underlying problem, because it feels like the regression is bad enough that it's worth simply un-regressing before addressing the deeper problem.
(In reply to :Gijs Kruitbosch from comment #12)
> Anyway, the patch is focused specifically on addressing the regression, not
> on the underlying problem, because it feels like the regression is bad
> enough that it's worth simply un-regressing before addressing the deeper
> problem.

(That and we're not so far away from 52 going to aurora, and this is a very safe patch, whereas I dunno how safe messing with the implementation of focus() is going to be.)
Tracking 52+ for this regression which affects the placement of the cursor.
tracking-firefox52: ? → +
Comment on attachment 8807854 [details]
Bug 1313403 - text (focus) goes to the wrong browser after foreground tab change,

https://reviewboard.mozilla.org/r/90854/#review90820
Attachment #8807854 - Flags: review?(mak77) → review+
Ni me to file 3 followups:

- probably should update the about blank creation that's in the context of the patch for bug 1000458 that I rebased around at the time and neglected to update
- focusing non-selected browsers should be a no-op as per the discussion in the comments here with Dão
- figure out if we can simplify this code as per IRC discussion with Marco. It's too hairy right now. Maybe we can split out the popup window case, or split some other stuff out to separate functions/methods/whatever.
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e388e453977c
text (focus) goes to the wrong browser after foreground tab change, r=mak
Depends on: 1315948
Comment on attachment 8807854 [details]
Bug 1313403 - text (focus) goes to the wrong browser after foreground tab change,

https://reviewboard.mozilla.org/r/90854/#review91232
Attachment #8807854 - Flags: review?(florian) → review+
Depends on: 1315950
Blocks: 1115009
(In reply to :Gijs Kruitbosch from comment #17)
> Ni me to file 3 followups:
> 
> - probably should update the about blank creation that's in the context of
> the patch for bug 1000458 that I rebased around at the time and neglected to
> update

bug 1315944

> - focusing non-selected browsers should be a no-op as per the discussion in
> the comments here with Dão

bug 1315950

> - figure out if we can simplify this code as per IRC discussion with Marco.
> It's too hairy right now. Maybe we can split out the popup window case, or
> split some other stuff out to separate functions/methods/whatever.

bug 1315948

waiting on a trypush with some green before relanding.
Flags: needinfo?(gijskruitbosch+bugs)
random drive-by review comment: There's the blessed browser.getTabBrowser() API that you can use instead of the more cumbersome browser.ownerGlobal.gBrowser.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2a80878603ad
text (focus) goes to the wrong browser after foreground tab change, r=florian,mak
(In reply to Dão Gottwald [:dao] from comment #23)
> random drive-by review comment: There's the blessed browser.getTabBrowser()
> API that you can use instead of the more cumbersome
> browser.ownerGlobal.gBrowser.

Fixed before landing. Also your point about "NB". Thanks!
https://hg.mozilla.org/mozilla-central/rev/2a80878603ad
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Verified as fixed with latest Nightly 52.0a1 (2016-11-10).
Thank you very much! \o/
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Depends on: 1316863
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: