Cory Price [:ckprice] (bugmail disabled, NI me!) Reporter
	Description • 8 years ago

In bug 1204836 a change was made to about:home which changed focus to the search bar when any searchable keypress occurred.

We have a handful of snippets which depend on user input. For instance, the mobile activation snippet[0].

This bug is opened to revert the work done in bug 1204836.

[0] http://cl.ly/351P361T3n3e

	Luís Miguel [:quicksaver] (not reading bugmail)
	Comment 1 • 8 years ago

I'd rather see it fixed than reverted completely. For instance, by adding a check at the top of keypress listener implemented to ignore when the cursor is already on a text/input field.

	Cory Price [:ckprice] (bugmail disabled, NI me!) Reporter
	Comment 2 • 8 years ago

Hi :gijs - what can we do to move this bug along?

	:Gijs (he/him) Assignee
	Comment 3 • 8 years ago

Ugh. I'm sorry I didn't think about this when I reviewed the original patch - I was not aware we had snippets with input fields.

As an aside, Cory, do we not have any automated tests that run with the current set of snippets and check that they behave properly against the current Firefox release and/or Nightly? It sounds like we don't, and that, just like tests for the self-support stuff that was repeatedly breaking stuff late last year, is something we really need for this and any other "we're a webserver and we're pushing code that runs as part of Firefox-the-product" ("go faster" and what have you). If we don't have such tests, Cory can you file a bug to get that done? If we do, why didn't we notice this before 43 got to release? The code changes landed on Nightly and were then uplifted to Aurora, but we basically had 2 months to notice this, and didn't. :-\

For this bug right now, I can take it, but is there a way for me to test any of the snippets locally?

(FWIW, I agree with Luís that we reverting just bug 1204836 isn't what we want here, if only because we'll also need to revert the other bug that added the ctrl-f shortcut instead of what we're doing now, and it will all just get really messy. We should just fix the code here to not "steal" keypresses from any inputboxes.)

Finally, I believe snippets can work around this by adding an event listener for the "keypress" event to the textbox and calling event.stopPropagation() on the event. That should probably be a separate bug against the snippets, though.

	Cory Price [:ckprice] (bugmail disabled, NI me!) Reporter
	Comment 4 • 8 years ago

:Gijs - this snippet was developed, tested and deployed pre-43. It was then disabled as other promotions were cycled in. We recently restarted the iOS promotion post-43, thus uncovered this issue.

This is an owned channel that is on a calendar of cycling promotions, any changes to it should consult/inform marketing. The changes in bug 1204836 were executed without the involvement of the channel owners, so I'm not sure how this could have been detected until we wanted to deploy a snippet which exposed it.

(In reply to :Gijs Kruitbosch from comment #3)
> Finally, I believe snippets can work around this by adding an event listener
> for the "keypress" event to the textbox and calling event.stopPropagation()
> on the event. That should probably be a separate bug against the snippets,
> though.

Bug 1238092 has been opened to address the immediate problem.

We have other snippets which utilize user input, and I'd rather not have to add this hack for everyone one we push out. What is the timeline to get this bug resolved?

	:Gijs (he/him) Assignee
	Comment 5 • 8 years ago

(In reply to Cory Price [:ckprice] from comment #4)
> :Gijs - this snippet was developed, tested and deployed pre-43. It was then
> disabled as other promotions were cycled in. We recently restarted the iOS
> promotion post-43, thus uncovered this issue.
> 
> This is an owned channel that is on a calendar of cycling promotions, any
> changes to it should consult/inform marketing. The changes in bug 1204836
> were executed without the involvement of the channel owners,

Marketing does not own literally all of about:home. Firefox and platform engineers make changes that affect it. Any change to Gecko could potentially affect it. It is not reasonable to "consult/inform marketing" whenever we make any change that could potentially affect about:home.

If we *had* told marketing "hi, we're going to make otherwise-useless keypresses automatically go into the search box to make it easier for users to search on about:home", would Marketing have objected? Likely not, right? There's no impact to them. Bugs aren't usually expected when we make changes...

This is why the snippets should be tested, ideally with every push to either mozilla-central or the snippet service or whatever runs them.

> so I'm not sure
> how this could have been detected until we wanted to deploy a snippet which
> exposed it.

Generally, when we turn features off on the Firefox end, but keep the code, it's done via a pref. Tests for those features continue to run: they turn the pref on as part of the test. Likewise, I would expect integrated tests to test snippets that are not part of the current cycle of snippets we serve to end-users.

> (In reply to :Gijs Kruitbosch from comment #3)
> > Finally, I believe snippets can work around this by adding an event listener
> > for the "keypress" event to the textbox and calling event.stopPropagation()
> > on the event. That should probably be a separate bug against the snippets,
> > though.
> 
> Bug 1238092 has been opened to address the immediate problem.
> 
> We have other snippets which utilize user input, and I'd rather not have to
> add this hack for everyone one we push out.

Of course. I'm just aware that this isn't serious enough to spin a 43.0.5 for, and 44 beta uplifts have been restricted by drivers, so I don't know that we can fix it for 44, either, and I assumed you wanted a fix sooner than 45. Just trying to help.

> What is the timeline to get this
> bug resolved?

I'm happy to spend time to fix this next week (it's past EOB here today) -- but I'll need to be able to reproduce, and this snippet isn't shown to everyone (and now, AIUI, to nobody, right?). Is there a way for me to test with the snippet? How?

	Cory Price [:ckprice] (bugmail disabled, NI me!) Reporter
	Comment 6 • 8 years ago

(In reply to :Gijs Kruitbosch from comment #5)
> If we *had* told marketing "hi, we're going to make otherwise-useless
> keypresses automatically go into the search box to make it easier for users
> to search on about:home", would Marketing have objected? Likely not, right?
Unfortunately, this will remain an unknown.

> This is why the snippets should be tested, ideally with every push to either
> mozilla-central or the snippet service or whatever runs them.
Testing every snippet with every code push to mozilla-central is something that could be hard to achieve. I'm also not convinced that automated testing could have detected this.

I'm hoping that - at the least - this conversation will bring more awareness to the fact that changes made to about:home do not exist in a vacuum.

> I'm happy to spend time to fix this next week (it's past EOB here today) --
> but I'll need to be able to reproduce, and this snippet isn't shown to
> everyone (and now, AIUI, to nobody, right?). Is there a way for me to test
> with the snippet? How?
Great, thanks! I'll work with you to get you access.

	:Gijs (he/him) Assignee
	Comment 7 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/30317/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30317/

	Luís Miguel [:quicksaver] (not reading bugmail)
	Comment 8 • 8 years ago

Just had a thought, if about:home has access to BrowserUtils.jsm in toolkit, it could probably use its .shouldFastFind method. I'm sure the focused element restrictions here are the same as the ones applied to fastfind, and the page location restrictions in that method shouldn't affect about:home (and those should/will be taken out of that method anyway for optimization, at least by bug 1218351 if that's ever fixed).

Unless I missed something or that's not optimal?

	:Gijs (he/him) Assignee
	Comment 9 • 8 years ago

(In reply to Luís Miguel [:quicksaver] from comment #8)
> Just had a thought, if about:home has access to BrowserUtils.jsm in toolkit,
> it could probably use its .shouldFastFind method. I'm sure the focused
> element restrictions here are the same as the ones applied to fastfind, and
> the page location restrictions in that method shouldn't affect about:home
> (and those should/will be taken out of that method anyway for optimization,
> at least by bug 1218351 if that's ever fixed).
> 
> Unless I missed something or that's not optimal?

Hmm. It's an interesting idea - I'd forgotten we had that code. But it's opt-out (that is, it will return "yes, fast find" by default, unless you match the items listed in it), and the patch I've written is essentially opt-in (that is, it will return "no, don't fast find" if it finds any element other than document/body is focused, with the exception of links, buttons and input type="button" / input type="submit").

OTOH, sharing code is good, having similar-purpose code in more than one place is bad, and normal text inputs should match the checks in shouldFastFind. I'll let the reviewers decide what they want me to do.

	Giorgos Logiotatidis [:giorgos]
	Comment 10 • 8 years ago

Gijs is there a linux build with your patch I can use to test the fix?

Also can we land this on beta and hit release on the 26th? If now which is the release schedule for this change?

Thanks!

	:Gijs (he/him) Assignee
	Comment 11 • 8 years ago

(In reply to Giorgos Logiotatidis [:giorgos] from comment #10)
> Gijs is there a linux build with your patch I can use to test the fix?

You should be able to use an artifact build (http://www.ncalexander.net/blog/2015/12/31/firefox-artifact-builds-for-mac/ - Linux support has landed now) and get a Firefox build together in under 5 minutes once you have the source tree. To apply my patch, you could either pull the rev listed on bugzilla from https://reviewboard-hg.mozilla.org/ and rebase on whatever tip you have, update, and run "./mach build faster" which will take <10 seconds. If you're not friends with hg, you could just save https://reviewboard-hg.mozilla.org/gecko/raw-rev/3edf85953149 and apply with diff.

Alternatively, wait for Linux builds on https://treeherder.mozilla.org/#/jobs?repo=try&revision=b78a4c59ad8d .

> Also can we land this on beta and hit release on the 26th?

That is unlikely, see https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.planning/jDiniugtwgU

> If now which is the release schedule for this change?

It needs to be reviewed and landed on Nightly first. Once it sticks I'll request uplift. At this point I see no reason for it not to make Firefox 45. As I noted in comment #3, there's a workaround snippets can use in the meantime.

	arni2033
	Comment 12 • 8 years ago

> 69      document.activeElement != document &&
Did you mean "document.activeElement != document.documentElement &&"

	Ben (:bensternthal)
	Comment 13 • 8 years ago

I am not qualified to be the reviewer on this. I am not at all familiar with the code in product and am not able to test this change locally.

	:Gijs (he/him) Assignee
	Comment 14 • 8 years ago

(In reply to Ben (:bensternthal) from comment #13)
> I am not qualified to be the reviewer on this. I am not at all familiar with
> the code in product and am not able to test this change locally.

Jared is "qualified" to be a reviewer here. Are neither you or Giorgios able to test this? Why not? I pointed to builds with the change in comment #11, and both of you are much more familiar with the snippets service than Jared or I are. Makes sense that one of you at least checks that the fix is sane...

(In reply to arni2033 from comment #12)
> > 69      document.activeElement != document &&
> Did you mean "document.activeElement != document.documentElement &&"

Huh, I guess though the spec ( https://html.spec.whatwg.org/multipage/interaction.html#dom-document-activeelement ) starts with "Let candidate be the Document on which the method was invoked.", step 5 is:

If candidate is a Document that has a body element, then let candidate be the body element of that Document.
Otherwise, if candidate is a Document that has a root element, then let candidate be the root element of that Document.
Otherwise, if candidate is a Document, then let candidate be null.

So presumably you never actually end up with candidate, and they might as well have specced it with candidate being null to begin with. :-\

Anyway, yes, I think it probably makes more sense to check for the document.documentElement. It doesn't really materially matter because about:home should (tm) always have a body. Perhaps I should take that check out altogether.

	arni2033
	Comment 15 • 8 years ago

When I click in searchbar, then press Tab - then the following returns true:
> document.documentElement == document.activeElement //true
while these two return false:
> document.body == document.activeElement            //false
> document == document.activeElement                 //false
document can't be "focused" at all. But you probably know better. That's all I wanted to add anyway

	Ben (:bensternthal)
	Comment 16 • 8 years ago

Gijs: I delegate to Giorgos to test :)

	:Gijs (he/him) Assignee
	Comment 17 • 8 years ago

Comment on attachment 8706328 [details]
MozReview Request: Bug 1237350 - don't steal focus for the search box if another piece of non-button/non-input UI has focus, r?jaws,giorgios

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30317/diff/1-2/

	:Gijs (he/him) Assignee
	Comment 18 • 8 years ago

Comment on attachment 8706328 [details]
MozReview Request: Bug 1237350 - don't steal focus for the search box if another piece of non-button/non-input UI has focus, r?jaws,giorgios

(In reply to Luís Miguel [:quicksaver] from comment #8)
> Just had a thought, if about:home has access to BrowserUtils.jsm in toolkit,

And I realized that about:home is unprivileged. So it doesn't. :-(

Fixed the documentElement check in this revision, thanks arni.

New try push:

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

	Jared Wein [:jaws] (please needinfo? me)
	Comment 19 • 8 years ago

Comment on attachment 8706328 [details]
MozReview Request: Bug 1237350 - don't steal focus for the search box if another piece of non-button/non-input UI has focus, r?jaws,giorgios

https://reviewboard.mozilla.org/r/30317/#review27521

	Giorgos Logiotatidis [:giorgos]
	Comment 20 • 8 years ago

(In reply to :Gijs Kruitbosch from comment #18)
> Comment on attachment 8706328 [details]
> MozReview Request: Bug 1237350 - don't steal focus for the search box if
> another piece of non-button/non-input UI has focus, r?jaws,giorgios
> 
> (In reply to Luís Miguel [:quicksaver] from comment #8)
> > Just had a thought, if about:home has access to BrowserUtils.jsm in toolkit,
> 
> And I realized that about:home is unprivileged. So it doesn't. :-(
> 
> Fixed the documentElement check in this revision, thanks arni.
> 
> New try push:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4188604dccda

Tried on linux, works as expected: I loaded the mobile activation snippets which has a text box and clicking on the snippets textbox and typing in works.

Thanks for the fix!

	Giorgos Logiotatidis [:giorgos]
	Comment 21 • 8 years ago

Comment on attachment 8706328 [details]
MozReview Request: Bug 1237350 - don't steal focus for the search box if another piece of non-button/non-input UI has focus, r?jaws,giorgios

https://reviewboard.mozilla.org/r/30317/#review27663

Tried linux build and works as expected: Textbox in snippet maintains focus and user can type in the text box.

	Pulsebot
	Comment 22 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/34360c913f6a

	:Gijs (he/him) Assignee
	Comment 23 • 8 years ago

Comment on attachment 8706328 [details]
MozReview Request: Bug 1237350 - don't steal focus for the search box if another piece of non-button/non-input UI has focus, r?jaws,giorgios

Approval Request Comment
[Feature/regressing bug #]: about:home snippets / bug 1204836
[User impact if declined]: broken snippets
[Describe test coverage new/current, TreeHerder]: about:home has test coverage, but not the snippets. The test coverage would catch this change breaking the general "undirected text input should go into the search field" feature, but not this not having fixed the snippets. Both me and Giorgos tested the snippets and this seems to be enough to fix them.
[Risks and why]: low, focused change in 1 file specific to all this, verified by 2 different people, has automated tests for the page generally
[String/UUID change made/needed]: no


I'm expecting beta approval to be denied, but one doesn't get if one doesn't ask. This is certainly annoying for the snippets team, and although there's a workaround, it'd be good if they can stop using that "soon".

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 24 • 8 years ago

Comment on attachment 8706328 [details]
MozReview Request: Bug 1237350 - don't steal focus for the search box if another piece of non-button/non-input UI has focus, r?jaws,giorgios

I am denying this based on the more stringent Beta44 uplift criteria that allows only for fixing critical regressions, sec and stability issues. To me this fix, does not meet that bar. Sorry!

Also, Gijs has proposed a workaround in comment 3, which is a good option for Fx44 end-users and this fix should be fixed and uplifted to Aurora45.

	Carsten Book [:Tomcat]
	Comment 25 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/34360c913f6a

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/5b08881ecf1c

	Mayur Patil
	Comment 27 • 8 years ago

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected