1359056 - Search panel jumps when hovering the One-off buttons

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Simona Badau, Desktop QA]

Attachment: SearchPanel.mp4

[Affected versions]:
- Nightly 55.0a1

[Affected platforms]:
- Windows, Ubuntu, and Mac OS X

[Steps to reproduce]:
1. Launch Firefox
2. Start typing in the Search Bar 
3. Enter Customize (Open Menu -> Customize)
4. Hover your mouse over the search panel


[Expected result]:
- After step 3 - the search panel should not be visible in Customize mode.
- After step 4 - the search panel along with the one-off bar and buttons should not jump.

[Actual result]:
- After step 3 - the search panel is visible in Customize mode
- After step 4 - the search panel along with the one-off bar and one-off buttons should not jump.
Please see the screencast for more details.

[Regression range]:
- I'm still investigating it, I'll post is as soon as possible.

Comment 1

[Simona Badau, Desktop QA]

Regression range:
Last good revision: 51df9eb411486666270d12120de7613d483bedd9
First bad revision: 4266cb39733da9b8316c613c7f03a91e693c893b
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=51df9eb411486666270d12120de7613d483bedd9&tochange=4266cb39733da9b8316c613c7f03a91e693c893b

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

I can reproduce this in 54 (Developer Edition), so this isn't a recent regression. The regression range from comment 1 seems unrelated at first glance. Simona, could you try to find a regression range in the 54 cycle?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

This is about one-offs in the search bar which shipped ages ago.

Comment 4

[Simona Badau, Desktop QA]

Indeed, I investigated a little and I was able to reproduce this issue even on Firefox 53 Release (Build ID: 20170413192749) but using some different steps.

The STR are:
1. Launch Firefox
2. Open a new tab and type something in the Search bar
3. Enter Customize Mode

Actual results:
The Search panel remains opened and on hovering the one-off buttons it jumps outside of the Firefox window.

I'll try tomorrow to narrow the regression of this issue.

Comment 5

[Simona Badau, Desktop QA]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #2)
> I can reproduce this in 54 (Developer Edition), so this isn't a recent
> regression. The regression range from comment 1 seems unrelated at first
> glance. Simona, could you try to find a regression range in the 54 cycle?

Last good revision: 2c8ce211041fa1e243fe30b03bbc966094a894c2
First bad revision: 11921c9f7d11e52db8da6f7dbaa9d6567227a3ad
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2c8ce211041fa1e243fe30b03bbc966094a894c2&tochange=11921c9f7d11e52db8da6f7dbaa9d6567227a3ad

Gijs, could you please take a look?

Comment 6

[:Gijs (he/him)]

I don't have cycles to investigate this in detail. The fix from that range that likely triggered something here is that we restore focus after a popup closes (such as the hamburger panel, when we enter customize mode). It seems to me the main issue here is how aggressively the search popup opens as soon as the search field receives focus in any way. This has been causing issues for years now, and I don't know how to fix that without altering that behaviour so we only open the popup when people actually (left) click on the search bar or explicitly move focus with the keyboard (or type in the search box), which seems like it'd make more sense (see also: bug 1190311). In principle, the behaviour of restoring focus to the search box when the hamburger panel closes is correct.

The only other thing I can think of is to ensure we close (or prevent opening, in popupshowing/popupshown handlers) the popup from within the search binding when customize mode opens (either based on an event or based on when the search binding itself is reinitialized because we wrap the element in a toolbarpaletteitem element). But that feels like a workaround for this specific issue.

I don't know why the panel sometimes (but not always?) loses its position / "jumps" if it's left open in customize mode. I wouldn't worry too much about fixing that over fixing the overarching issue that the popup is left open.

Based on the regression range, we already shipped this in 52 and 53. I also can reproduce only intermittently - sometimes, apparently, customize mode kicks in before the search popup reshows (having disappeared when the search field lost focus in favour of the main hamburger panel), and sometimes it does not, so at the moment it doesn't seem to make sense to me to spend too much time on this.

Panos, what do you think?

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

I agree with the above analysis, with the sole exception that I can always reproduce this in both macOS and Windows. I would gladly take the suggested workaround just to avoid this glaring issue. I'll see if I can find someone to work on this.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Bug 1359056 - Don't open the search panel when entering customization mode.

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

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8867466 [details]
Bug 1359056 - Don't open the search panel when entering customization mode.

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

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a15c5e98033db6b62b474cf49d7e4d40602ed25

Comment 11

[:Gijs (he/him)]

Comment on attachment 8867466 [details]
Bug 1359056 - Don't open the search panel when entering customization mode.

https://reviewboard.mozilla.org/r/139014/#review143036

::: commit-message-96b36:1
(Diff revision 2)
> +Don't open the search panel when entering customization mode (bug 1359056). r?Gijs

Nit: bug at the start please

::: browser/components/search/test/browser_searchbar_openpopup.js:45
(Diff revision 2)
> +function endCustomizing(aWindow = window) {
> +  if (aWindow.document.documentElement.getAttribute("customizing") != "true") {
> +    return true;
> +  }
> +  Services.prefs.setBoolPref("browser.uiCustomization.disableAnimation", true);
> +  return new Promise(resolve => {
> +    function onCustomizationEnds() {
> +      Services.prefs.setBoolPref("browser.uiCustomization.disableAnimation", false);
> +      aWindow.gNavToolbox.removeEventListener("aftercustomization", onCustomizationEnds);
> +      resolve();
> +    }
> +    aWindow.gNavToolbox.addEventListener("aftercustomization", onCustomizationEnds);
> +    aWindow.gCustomizeMode.exit();
> +
> +  });
> +}

Please replace:

```js
async function endCustomizing(aWindow = window) {
  if (aWindow.document.documentElement.getAttribute("customizing") != "true") {
    return true;
  }
  await SpecialPowers.pushPrefEnv({set: [["browser.uiCustomization.disableAnimation", true]]});
  let eventPromise = BrowserTestUtils.waitForEvent(win.gNavToolbox, "aftercustomization");
  aWindow.gCustomizeMode.exit();
  return eventPromise;
}
```

::: browser/components/search/test/browser_searchbar_openpopup.js:62
(Diff revision 2)
> +function startCustomizing(aWindow = window) {
> +  if (aWindow.document.documentElement.getAttribute("customizing") == "true") {
> +    return null;
> +  }
> +  Services.prefs.setBoolPref("browser.uiCustomization.disableAnimation", true);
> +  return new Promise(resolve => {
> +    function onCustomizing() {
> +      aWindow.gNavToolbox.removeEventListener("customizationready", onCustomizing);
> +      Services.prefs.setBoolPref("browser.uiCustomization.disableAnimation", false);
> +      resolve();
> +    }
> +    aWindow.gNavToolbox.addEventListener("customizationready", onCustomizing);
> +    aWindow.gCustomizeMode.enter();
> +  });

```js
async function startCustomizing(aWindow = window) {
  if (aWindow.document.documentElement.getAttribute("customizing") == "true") {
    return true;
  }
  await SpecialPowers.pushPrefEnv({set: [["browser.uiCustomization.disableAnimation", true]]});
  let eventPromise = BrowserTestUtils.waitForEvent(win.gNavToolbox, "customizationready");
  aWindow.gCustomizeMode.enter();
  return eventPromise;
}
```

::: browser/components/search/test/browser_searchbar_openpopup.js:580
(Diff revision 2)
> +  let sawPopup = false;
> +  function listener() {
> +    sawPopup = true;
> +  }

Nit: please move this further down to where we're using it, right after the info("Entering customization mode")

::: browser/components/search/test/browser_searchbar_openpopup.js:592
(Diff revision 2)
> +  let shownPromise = promisePanelShown();
> +  PanelUI.show();

just:

```js
await PanelUI.show();
```

will work. Then we can remove the promisePanelShown() stuff from head.js

At this point, should we ensure that the searchPopup has hidden?

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8867466 [details]
Bug 1359056 - Don't open the search panel when entering customization mode.

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

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Gijs from comment #11)
> will work. Then we can remove the promisePanelShown() stuff from head.js

I assumed you meant remove it from the top of the file, which I did.

> At this point, should we ensure that the searchPopup has hidden?

Probably not important as startCustomizing that triggers hiding the popup and endCustomizing that follows aren't exactly snappy, but I did it just in case.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1c4ad36a95f
Don't open the search panel when entering customization mode. r=Gijs

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a1c4ad36a95f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. Seems edge-case enough that we can skip uplifting to ESR52, but given what a tiny patch it is, feel free to set the status back to affected and request approval if you feel otherwise.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8867466 [details]
Bug 1359056 - Don't open the search panel when entering customization mode.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1286389
[User impact if declined]: annoying visual glitch when entering customization mode after having opened the search panel
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: I don't think it's necessary
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: minor risk, it's a small 2-line fix
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 18

[Sylvestre Ledru [:Sylvestre]]

The patch doesn't apply to beta:

merging browser/components/search/content/search.xml
merging browser/components/search/test/browser_searchbar_openpopup.js
 warning: conflicts while merging browser/components/search/test/browser_searchbar_openpopup.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8867466 [details]
Bug 1359056 - Don't open the search panel when entering customization mode.

Fix an annoying visual glitch related to search panel and include test. Beta54+. Should be in 54 beta 10.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out of beta for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=101012097&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/64096bfc5a89

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch for beta

Beta doesn't have the async/await test changes, so I'm now using the old style generator functions.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Pushed: https://hg.mozilla.org/releases/mozilla-beta/rev/16c20bf328da

Comment 23

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 53 (2017.04.13) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 54.0b10 and 55.0a1 on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.10.