Closed Bug 1359056 Opened 3 years ago Closed 3 years ago

Search panel jumps when hovering the One-off buttons

Categories

(Firefox :: Search, defect, P1)

52 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- verified
firefox55 --- verified

People

(Reporter: simona.marcu, Assigned: past)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(3 files)

Attached video 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.
Regression range:
Last good revision: 51df9eb411486666270d12120de7613d483bedd9
First bad revision: 4266cb39733da9b8316c613c7f03a91e693c893b
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=51df9eb411486666270d12120de7613d483bedd9&tochange=4266cb39733da9b8316c613c7f03a91e693c893b
Whiteboard: [fxsearch]
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?
Flags: needinfo?(simona.marcu)
Priority: -- → P1
This is about one-offs in the search bar which shipped ages ago.
No longer blocks: 1337003
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.
(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?
Flags: needinfo?(simona.marcu) → needinfo?(gijskruitbosch+bugs)
Version: 55 Branch → 52 Branch
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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(past)
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.
Blocks: 1286389
Flags: needinfo?(past)
Assignee: nobody → past
Status: NEW → ASSIGNED
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?
Attachment #8867466 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1c4ad36a95f
Don't open the search panel when entering customization mode. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/a1c4ad36a95f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.
Flags: needinfo?(past)
Flags: in-testsuite+
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]:
Flags: needinfo?(past)
Attachment #8867466 - Flags: approval-mozilla-beta?
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
Flags: needinfo?(past)
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.
Attachment #8867466 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Attached patch Patch for betaSplinter Review
Beta doesn't have the async/await test changes, so I'm now using the old style generator functions.
Flags: needinfo?(past)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.