[photon] Move the search bar in the customization palette for new profiles

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: past, Assigned: Paolo)

Tracking

(Depends on 3 bugs, Blocks 1 bug)

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [fxsearch][small linux-qr tresize regression expected])

Attachments

(1 attachment)

The Photon design spec calls for a unified location and search bar. We have to make similar changes as in bug 694291 at least for the new profile case.

There are experiments underway to identify the right set of profiles to receive this experience, so the scope may change a bit soon.
Also, the location of the search bar (navbar vs. customization) has to be controlled by a checkbox in about:preferences.
Summary: Move the search bar in the customization palette for new profiles → [photon] Move the search bar in the customization palette for new profiles
Assignee

Comment 2

2 years ago
(In reply to Panos Astithas [:past] (56 Regression Engineering Owner) (please ni?) from comment #1)
> Also, the location of the search bar (navbar vs. customization) has to be
> controlled by a checkbox in about:preferences.

Do you know if this is still a requirement, and if so could we move this change to a separate bug?

The position and presence of the search bar can already be changed easily in Customize mode.
Flags: needinfo?(past)
Javaun mentioned the checkbox earlier today, so I believe it is still a requirement, but I don't see why we couldn't do it in a followup.

Latest tweak to the scope of profiles receiving the unified treatment: new profiles and users who have already moved the search bar to the customization palette or the overflow menu. I guess this is rather obvious, but I forgot to mention it in the bug description.
Flags: needinfo?(past)
Assignee

Updated

2 years ago
Blocks: 1393437
Assignee

Updated

2 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Assignee

Comment 4

2 years ago
Some preliminary try builds to see the performance impact and which other regression tests need to be updated:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f60c3c54df3423b4dc73e22a194ee66481153553
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ed9a6281116372eccaed3e3725afe06b663c30c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=80ce288edf8499a7b03ff63cdad8b6c8b1d63846

Interestingly enough, it seems that just changing the default placement without any special migration step actually makes new profiles use the unified bar, while updated profiles keep the separate search bar. I'd have expected that to be the case only for profiles that had been customized, but it seems to work also for brand new profiles created without the patch.
Assignee

Comment 5

2 years ago
(In reply to :Paolo Amadini from comment #4)
> Interestingly enough, it seems that just changing the default placement
> without any special migration step actually makes new profiles use the
> unified bar, while updated profiles keep the separate search bar. I'd have
> expected that to be the case only for profiles that had been customized, but
> it seems to work also for brand new profiles created without the patch.

Gijs, does that sound like a reliable assumption based on how CustomizableUI works?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #5)
> (In reply to :Paolo Amadini from comment #4)
> > Interestingly enough, it seems that just changing the default placement
> > without any special migration step actually makes new profiles use the
> > unified bar, while updated profiles keep the separate search bar. I'd have
> > expected that to be the case only for profiles that had been customized, but
> > it seems to work also for brand new profiles created without the patch.
> 
> Gijs, does that sound like a reliable assumption based on how CustomizableUI
> works?

No. In theory, we shouldn't need to save customized state if there's no customization in the toolbar. That we save some things anyway has come up before (cf. https://bugzilla.mozilla.org/show_bug.cgi?id=956731#c6 and later), but that's a bug, and not something you should depend on. I don't know why it would be the case for the nav-bar, too, especially not now that nothing (I think) is manipulating the defaults anymore, now that screenshots is no longer adding any toolbar buttons. In fact, the screenshots thing might be why - I think we're still adding a browser action but then hiding it permanently... :-\

Anyway, once that gets tidied up (for the first version of screenshots that doesn't target 56, which in turn may end up shipping with 57), there shouldn't be any reason to save navbar state and as a result I wouldn't expect it to be saved, and if that were true it would break your assumptions.

Note also that "just" changing the default placement would mean that if anybody on a newer profile picks "restore defaults", they will get the new experience. I don't know if that's desired, it's not clear from the descriptions in this bug so far.


Finally, I am confused about comment #3:

(In reply to Panos Astithas [:past] (56 Regression Engineering Owner) (please ni?) from comment #3)
> Latest tweak to the scope of profiles receiving the unified treatment: new
> profiles and users who have already moved the search bar to the
> customization palette or the overflow menu. I guess this is rather obvious,
> but I forgot to mention it in the bug description.

What is the suggestion for what happens to people who have moved it to the overflow panel? Note that we have already done work that allows you to use it that way by using the relevant shortcut, and people used to rely on this in the old hamburger panel (from which we'll auto-move the search bar to the overflow panel). So I wouldn't want those users have the search bar just end up in the palette...
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs (queue backed up, slow) from comment #6)
> (In reply to Panos Astithas [:past] (56 Regression Engineering Owner)
> (please ni?) from comment #3)
> > Latest tweak to the scope of profiles receiving the unified treatment: new
> > profiles and users who have already moved the search bar to the
> > customization palette or the overflow menu. I guess this is rather obvious,
> > but I forgot to mention it in the bug description.
> 
> What is the suggestion for what happens to people who have moved it to the
> overflow panel? Note that we have already done work that allows you to use
> it that way by using the relevant shortcut, and people used to rely on this
> in the old hamburger panel (from which we'll auto-move the search bar to the
> overflow panel). So I wouldn't want those users have the search bar just end
> up in the palette...

Sorry, I meant to say "overflow panel" instead of "overflow menu" in comment 3. Indeed, we don't want to undo the customization for users who have already customized their search bars away.
Assignee

Comment 8

2 years ago
(In reply to :Gijs (queue backed up, slow) from comment #6)
> Note also that "just" changing the default placement would mean that if
> anybody on a newer profile picks "restore defaults", they will get the new
> experience. I don't know if that's desired, it's not clear from the
> descriptions in this bug so far.

Yes, I think this is the intended behavior. Restoring the defaults manually assumes the user already knows about customize mode, and may be able to place the widget back if necessary. For auto-reset of very old profiles, we likely want to offer the unified bar as well.

> (In reply to Panos Astithas [:past] (56 Regression Engineering Owner)
> (please ni?) from comment #3)
> > Latest tweak to the scope of profiles receiving the unified treatment: new
> > profiles and users who have already moved the search bar to the
> > customization palette or the overflow menu. I guess this is rather obvious,
> > but I forgot to mention it in the bug description.
> 
> What is the suggestion for what happens to people who have moved it to the
> overflow panel?

I think unified treatment just means that the main user interface will not show the search bar, but I assume the keyboard shortcuts will continue to work like they do today for the overflow panel case. I'm keeping the existing regression tests for these cases.
Assignee

Updated

2 years ago
Depends on: 1395983
Assignee

Comment 9

2 years ago
(In reply to :Gijs (queue backed up, slow) from comment #6)
> that's a bug, and not something you should depend on. I don't
> know why it would be the case for the nav-bar, too, especially not now that
> nothing (I think) is manipulating the defaults anymore, now that screenshots
> is no longer adding any toolbar buttons. In fact, the screenshots thing
> might be why - I think we're still adding a browser action but then hiding
> it permanently... :-\

I filed bug 1395983 to fix this, and I noticed that this is caused by the hookDeveloperToggle method that registers the developer tools widget.

> Anyway, once that gets tidied up (for the first version of screenshots that
> doesn't target 56, which in turn may end up shipping with 57), there
> shouldn't be any reason to save navbar state and as a result I wouldn't
> expect it to be saved, and if that were true it would break your assumptions.

So, the behavior of createWidget causing the state to be saved has been around since the beginning, and the developer tools dynamic widget has been around since at least Firefox 48 in bug 1261920. I think this means we can expect any profile not older than Firefox 48 to have saved customization state, right? I did test a new profile on Firefox 48 and it did save state immediately.

If the above is correct, and from a product perspective we deem acceptable that users who have not used Firefox since version 47 _may_ end up with a unified bar, it would simplify the upgrade logic a lot and save us quite some time. I was running into edge cases with lazy intialization of the customizable areas that I can just ignore completely if we don't need a migration step.
No longer depends on: 1395983
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #9)
> (In reply to :Gijs (queue backed up, slow) from comment #6)
> > that's a bug, and not something you should depend on. I don't
> > know why it would be the case for the nav-bar, too, especially not now that
> > nothing (I think) is manipulating the defaults anymore, now that screenshots
> > is no longer adding any toolbar buttons. In fact, the screenshots thing
> > might be why - I think we're still adding a browser action but then hiding
> > it permanently... :-\
> 
> I filed bug 1395983 to fix this, and I noticed that this is caused by the
> hookDeveloperToggle method that registers the developer tools widget.
> 
> > Anyway, once that gets tidied up (for the first version of screenshots that
> > doesn't target 56, which in turn may end up shipping with 57), there
> > shouldn't be any reason to save navbar state and as a result I wouldn't
> > expect it to be saved, and if that were true it would break your assumptions.
> 
> So, the behavior of createWidget causing the state to be saved has been
> around since the beginning, and the developer tools dynamic widget has been
> around since at least Firefox 48 in bug 1261920. I think this means we can
> expect any profile not older than Firefox 48 to have saved customization
> state, right? I did test a new profile on Firefox 48 and it did save state
> immediately.
> 
> If the above is correct, and from a product perspective we deem acceptable
> that users who have not used Firefox since version 47 _may_ end up with a
> unified bar, it would simplify the upgrade logic a lot and save us quite
> some time. I was running into edge cases with lazy intialization of the
> customizable areas that I can just ignore completely if we don't need a
> migration step.

You can just add a migration step inside CustomizableUI.jsm , like I did in the downloads button case. That should work either way and be more reliable, even if we stop storing state for non-customized toolbars.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 11

2 years ago
(In reply to :Gijs (queue backed up, slow) from comment #10)
> You can just add a migration step inside CustomizableUI.jsm , like I did in
> the downloads button case. That should work either way and be more reliable,
> even if we stop storing state for non-customized toolbars.

Except we should "prime" that migration step in nsBrowserGlue.js because it depends on whether the non-customized (hence unversioned) state was created on a profile before or after Firefox 56, doing that in _introduceNewBuiltinWidgets seems to be too early as we haven't defined the areas, and most of the existing logic assumes that gSavedState is present in the first place.

I started implementing something in SearchWidgetTracker but eventually I realized I was probably spending my time on a problem that we don't really have.
Assignee

Comment 12

2 years ago
For the rest, I've fixed most of the failures of tests that depended on the search bar being present by default.

I only had some remaining issues with the toolbar overflow tests, let's try to reduce the window size some more:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=095611d691a72be41326fd0a6bd925264a061d60
Comment hidden (mozreview-request)
Assignee

Comment 14

2 years ago
In preliminary Talos tests I've seen many small improvements, probably because we don't need to paint the search bar anymore, but a strange regression in "linux-qr". I've started new builds with more reruns, the results should appear here when ready:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=72efa487393444a6ae6c0836cbb7cb994ad00248&newProject=try&newRevision=41bd6918b93fe704504faa9aa461f3f1b6a18ba1&framework=1
Assignee

Comment 15

2 years ago
mozreview-review
Comment on attachment 8903663 [details]
Bug 1387416 - Place the search bar in the customization palette for new profiles.

https://reviewboard.mozilla.org/r/175434/#review180556

::: browser/components/customizableui/test/browser_923857_customize_mode_event_wrapping_during_reset.js
(Diff revision 1)
>    let downloadsButton = document.getElementById("downloads-button");
> -  let searchBox = document.getElementById("search-container");
>    let palette = document.getElementById("customization-palette");
> -  ok(devButton && downloadsButton && searchBox && palette, "Stuff should exist");
> +  ok(devButton && downloadsButton && palette, "Stuff should exist");
>    simulateItemDrag(devButton, downloadsButton);
> -  simulateItemDrag(searchBox, palette);

Hm, maybe this should have picked another element to move to the palette?
(In reply to :Paolo Amadini from comment #8)
> (In reply to :Gijs (queue backed up, slow) from comment #6)
> > (In reply to Panos Astithas [:past] (56 Regression Engineering Owner)
> > (please ni?) from comment #3)
> > > Latest tweak to the scope of profiles receiving the unified treatment: new
> > > profiles and users who have already moved the search bar to the
> > > customization palette or the overflow menu. I guess this is rather obvious,
> > > but I forgot to mention it in the bug description.
> > 
> > What is the suggestion for what happens to people who have moved it to the
> > overflow panel?
> 
> I think unified treatment just means that the main user interface will not
> show the search bar, but I assume the keyboard shortcuts will continue to
> work like they do today for the overflow panel case. I'm keeping the
> existing regression tests for these cases.

This doesn't answer the question I have, it just complicates things further. What is "main user interface"? To me, the overflow menu is part of the "main user interface"...

My question is very simple: if a user upgrades to 57, and previously had the search bar in the hamburger panel, where is the search bar in 57?

Comment #3 reads to me like we would remove it from the overflow panel (where it would currently, without this patch, live). Your comment, in my reading, seems to corroborate that reading, but is still not explicit.

I disagree that this is the right thing to do - if users have customized the search bar then it should be kept in the location they put it in, and if they put it in the hamburger panel it should migrate to the overflow panel as do all non-default items from the old hamburger panel (see bug 1354117).

I don't know if I'm just misunderstanding both your comments or whether you both actually believe we should remove the bar from the overflow panel.
Flags: needinfo?(paolo.mozmail)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8903663 [details]
Bug 1387416 - Place the search bar in the customization palette for new profiles.

https://reviewboard.mozilla.org/r/175434/#review180728

::: browser/app/profile/firefox.js:413
(Diff revision 1)
>  
>  // comma seperated list of of engines to hide in the search panel.
>  pref("browser.search.hiddenOneOffs", "");
>  
> -// Mirrors whether the search-container widget is in the navigation toolbar. The
> -// default value of this preference must match the DEFAULT_AREA_PLACEMENTS of
> +// Mirrors whether the search-container widget is in the navigation toolbar.
> +pref("browser.search.widget.inNavBar", false);

Why do we need this pref? I wasn't involved in the patches that added this, so I don't understand what purpose it serves. It looks to me like it should be possible to just read the position of the search widget where code would otherwise read the pref...

::: browser/base/content/browser.xul:1165
(Diff revision 1)
>          </menupopup>
>        </toolbarbutton>
> +
> +      <toolbaritem id="search-container" title="&searchItem.title;"
> +                   align="center" class="chromeclass-toolbar-additional panel-wide-item"
> +                   cui-areatype="toolbar"

You shouldn't keep cui-areatype...

::: browser/base/content/browser.xul:1166
(Diff revision 1)
>        </toolbarbutton>
> +
> +      <toolbaritem id="search-container" title="&searchItem.title;"
> +                   align="center" class="chromeclass-toolbar-additional panel-wide-item"
> +                   cui-areatype="toolbar"
> +                   flex="100" persist="width" removable="true">

You don't need 'removable=true' when it's in the palette, CUI automatically adds it because obviously, if it can be in the palette, it is removable...

::: browser/base/content/test/about/browser_aboutHome.js:486
(Diff revision 1)
>  });
>  
>  add_task(async function() {
>    info("Cmd+k should focus the search box in the toolbar when it's present");
>  
> +  Services.prefs.setBoolPref("browser.search.widget.inNavBar", true);

This should use customizableUI to add/remove the widget. I don't understand what purpose the pref serves, but in any case I assume it's temporary and we don't want this pref forever? It's very confusing... just use CUI APIs.

::: browser/components/customizableui/SearchWidgetTracker.jsm:40
(Diff revision 1)
> +    // The placement of the widget always takes priority, and the preference
> +    // should always match the actual placement when the browser starts up.
> +    this.syncPreferenceWithWidget();

I continue to find this confusing... at first I thought the pref was a read-only mirroring of where the searchbar was, but now it seems it's not, and it can also be used to move the search bar around... but why? And from the looks of it, this won't do the right thing if the pref is changed before this module inits (the pref will get changed to the CUI position), for example by a shield pref flip that gets executed early on startup... Are there other callsites for syncWidgetWithPreference() that I'm missing? I don't see anything in DXR.

::: browser/components/customizableui/test/browser_901207_searchbar_in_panel.js:51
(Diff revision 1)
> +// The following tests require the search bar in the navigation toolbar.
> +add_task(async function() {
> +  await SpecialPowers.pushPrefEnv({ set: [
> +    ["browser.search.widget.inNavBar", true],
> +  ]});
> +});

Again, I would much prefer this used CUI, here and elsewhere.

::: browser/components/customizableui/test/browser_923857_customize_mode_event_wrapping_during_reset.js
(Diff revision 1)
>    let downloadsButton = document.getElementById("downloads-button");
> -  let searchBox = document.getElementById("search-container");
>    let palette = document.getElementById("customization-palette");
> -  ok(devButton && downloadsButton && searchBox && palette, "Stuff should exist");
> +  ok(devButton && downloadsButton && palette, "Stuff should exist");
>    simulateItemDrag(devButton, downloadsButton);
> -  simulateItemDrag(searchBox, palette);

Yes.

::: browser/components/uitour/test/browser_UITour_availableTargets.js:40
(Diff revision 1)
> +  // Note that the query function for the "search" target will throw if it's not
> +  // found. Since the search bar is not present by default, this is implicitly
> +  // testing that the callback still fires with the other available targets.

I think you should instead update the query function to not throw but return null, which is what it should be doing.

::: browser/modules/BrowserUITelemetry.jsm
(Diff revision 1)
>        "back-button",
>        "forward-button",
>        "stop-reload-button",
>        "home-button",
>        "urlbar-container",
> -      "search-container",

You need to add this to the palette list instead.
Attachment #8903663 - Flags: review?(gijskruitbosch+bugs) → review-
Assignee

Comment 18

2 years ago
(In reply to :Gijs (queue backed up, slow) from comment #16)
> My question is very simple: if a user upgrades to 57, and previously had the
> search bar in the hamburger panel, where is the search bar in 57?

In the overflow panel.

> Comment #3 reads to me like we would remove it from the overflow panel
> (where it would currently, without this patch, live). Your comment, in my
> reading, seems to corroborate that reading, but is still not explicit.

We definitely read comment 3 differently, but comment 8 is also technically incorrect. As implemented now, the unified treatment means that the search bar is not visible in the _navigation toolbar_ when you open a new window. It does not necessarily mean that the search bar is moved to the palette, because it can be located in any other customizable area, but neither that it is invisible. If the user moved the search bar to the tab bar then we leave it there, and this is considered to be a unified treatment despite the search bar is still visible.

There will be a checkbox in the preferences that mirrors the presence of the search bar in the navigation toolbar, see bug 1393437. This is linked to the definition of unified treatment, positively or negatively depending on the wording that will be chosen. Unless we change this definition, when the search bar is in the tab bar the checkbox will be in the opposite state than when the search bar is in the navigation toolbar.
Flags: needinfo?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #18)
> (In reply to :Gijs (queue backed up, slow) from comment #16)
> > My question is very simple: if a user upgrades to 57, and previously had the
> > search bar in the hamburger panel, where is the search bar in 57?
> 
> In the overflow panel.

Good! :-)

> > Comment #3 reads to me like we would remove it from the overflow panel
> > (where it would currently, without this patch, live). Your comment, in my
> > reading, seems to corroborate that reading, but is still not explicit.
> 
> We definitely read comment 3 differently, but comment 8 is also technically
> incorrect. As implemented now, the unified treatment means that the search
> bar is not visible in the _navigation toolbar_ when you open a new window.
> It does not necessarily mean that the search bar is moved to the palette,
> because it can be located in any other customizable area, but neither that
> it is invisible. If the user moved the search bar to the tab bar then we
> leave it there, and this is considered to be a unified treatment despite the
> search bar is still visible.
> 
> There will be a checkbox in the preferences that mirrors the presence of the
> search bar in the navigation toolbar, see bug 1393437. This is linked to the
> definition of unified treatment, positively or negatively depending on the
> wording that will be chosen. Unless we change this definition, when the
> search bar is in the tab bar the checkbox will be in the opposite state than
> when the search bar is in the navigation toolbar.

Ah, OK. That explains the pref. I'm... not thrilled about this, but OK.

As you suggest, we'll need to be careful about the wording. This is especially messy because all the other toolbars besides the main navigation toolbar can be hidden in a window based on website and/or user request (users can toggle the menubar and bookmarks toolbar, and the tabstrip gets hidden in chromeless popup windows, and things get even messier in fullscreen).
Assignee

Comment 20

2 years ago
(In reply to :Gijs (queue backed up, slow) from comment #17)
> Why do we need this pref? I wasn't involved in the patches that added this,
> so I don't understand what purpose it serves. It looks to me like it should
> be possible to just read the position of the search widget where code would
> otherwise read the pref...

Well, we could avoid relying on the about:config preference in our internal code, and we may be able to remove the preference entirely, but I think I would slightly prefer keeping the SearchWidgetTracker and also use the preference from tests. We could file a follow-up to change this implementation detail in a future version, when we're more confident we'll not need to push more pref-flipping experiments. What do you think?

I don't know if Mark relied on the preference for the work in progress patch for bug 1393437, but he can also look into using the CustomizableUI state directly.

> This should use customizableUI to add/remove the widget. I don't understand
> what purpose the pref serves, but in any case I assume it's temporary and we
> don't want this pref forever? It's very confusing... just use CUI APIs.

The positioning logic that is currently in SearchWidgetTracker is simple enough that it can probably be decentralized, on the other hand if we have the preference we might as well use it from the tests so we don't have to copy and paste the code. I was also considering writing a CustomizableUITestUtils helper, but it's probably overkill compared to copy and paste.

> I continue to find this confusing... at first I thought the pref was a
> read-only mirroring of where the searchbar was, but now it seems it's not,
> and it can also be used to move the search bar around... but why? And from
> the looks of it, this won't do the right thing if the pref is changed before
> this module inits (the pref will get changed to the CUI position), for
> example by a shield pref flip that gets executed early on startup...

This preference was indeed created to allow pref-flipping experiments, see bug 694291 comment 6 and below, and the choice to ignore changes during early startup was made to avoid accidentally moving the search bar on profiles migrating across versions. This was also mentioned in the experiment implementation bug 1390584.

> ::: browser/components/uitour/test/browser_UITour_availableTargets.js:40
> (Diff revision 1)
> > +  // Note that the query function for the "search" target will throw if it's not
> > +  // found. Since the search bar is not present by default, this is implicitly
> > +  // testing that the callback still fires with the other available targets.
> 
> I think you should instead update the query function to not throw but return
> null, which is what it should be doing.

This is an internal implementation detail of the module, isn't it? This is just testing that UITour.getConfiguration works, and I think the original comment just expressed the reason why the explicit test without the search bar was added in the first place. I retained the comment for completeness, but I guess I can just remove it now that the search bar is absent by default.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #20)
> (In reply to :Gijs (queue backed up, slow) from comment #17)
> > Why do we need this pref? I wasn't involved in the patches that added this,
> > so I don't understand what purpose it serves. It looks to me like it should
> > be possible to just read the position of the search widget where code would
> > otherwise read the pref...
> 
> Well, we could avoid relying on the about:config preference in our internal
> code, and we may be able to remove the preference entirely, but I think I
> would slightly prefer keeping the SearchWidgetTracker and also use the
> preference from tests. We could file a follow-up to change this
> implementation detail in a future version, when we're more confident we'll
> not need to push more pref-flipping experiments. What do you think?

I think the abstraction through the pref is something that will be harder to remove once we start relying on it from tests, and that if ultimately, the only reason for the pref to exist is for the toggle in the preferences, we might as well not have the preference and just make ticking / unticking the checkbox call CUI APIs directly. That then removes the need for all the pref vs. CUI tracking machinery, and removes potential for bugs where they could be out of sync, plus we wouldn't store the same information twice. We would keep a searchwidgettracker-like thing to make it a 1-line change to add/remove the search bar, or to check whether it's present, but I think that would ultimately reduce complexity.

We could do that in a followup bug if you think that makes more sense, but then we'll be updating all these tests again... though I suppose maybe half the work is figuring out which tests need updating, in which case maybe it doesn't matter so much.

> > ::: browser/components/uitour/test/browser_UITour_availableTargets.js:40
> > (Diff revision 1)
> > > +  // Note that the query function for the "search" target will throw if it's not
> > > +  // found. Since the search bar is not present by default, this is implicitly
> > > +  // testing that the callback still fires with the other available targets.
> > 
> > I think you should instead update the query function to not throw but return
> > null, which is what it should be doing.
> 
> This is an internal implementation detail of the module, isn't it? This is
> just testing that UITour.getConfiguration works, and I think the original
> comment just expressed the reason why the explicit test without the search
> bar was added in the first place. I retained the comment for completeness,
> but I guess I can just remove it now that the search bar is absent by
> default.

Well, I'm confused, I think.

https://dxr.mozilla.org/mozilla-central/rev/37824bf5c5b08afa7e689fceb935b8f457ebd9eb/browser/components/uitour/UITour.jsm#180-185 suggests that this is just a selector. I assumed that the comment meant there was a custom query function that somehow threw a JS error. Maybe there was, once upon a time, and there isn't anymore? Anyway, re-reading this, yes, I think we can just remove the comment.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 22

2 years ago
(In reply to :Gijs (queue backed up, slow) from comment #21)
> We could do that in a followup bug if you think that makes more sense, but
> then we'll be updating all these tests again... though I suppose maybe half
> the work is figuring out which tests need updating, in which case maybe it
> doesn't matter so much.

I would go for moving this to a P2 follow-up, because storing the information once makes more sense, but in the immediate I'd like to focus on landing the more risky changes.

Speaking of which, there is a confirmed Talos regression on "tresize" (optimized, e10s) on the Quantum Render build:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=72efa487393444a6ae6c0836cbb7cb994ad00248&newProject=try&newRevision=41bd6918b93fe704504faa9aa461f3f1b6a18ba1&framework=1

(Tick "Show only important changes" to filter it.)

Panos, do you know what is the current state of Quantum Render and the current policy for handling performance regressions? Is there anyone we could ask for an opinion? The only reason I can imagine for a performance regression is that we need to render a wider surface in one go, but this looks like the kind of issue for which identifying the real cause would take a very long time, and it may well turn out to be a measurement artifact.

Most other tests see definite performance improvements, most probably because of the reduction of DOM nodes and the number of input fields and surfaces to render. Windows tests show more improvement, but if I remember correctly this is because on Windows the changes are prone to triggering "steps" due to the lower resolution of the timers and the interaction with other events.
Flags: needinfo?(past)
Milan, do we care at this point about Talos regressions with Quantum Render? If we do, can we get somebody from that team to help us understand what is the problem here?
Flags: needinfo?(past) → needinfo?(milan)
Assignee

Comment 24

2 years ago
(In reply to :Gijs (queue backed up, slow) from comment #17)
> You shouldn't keep cui-areatype...
> You don't need 'removable=true'

I also fixed the "bookmarks-menu-button" while I was at it.
(In reply to :Gijs (queue backed up, slow) from comment #21)
> I think the abstraction through the pref is something that will be harder to
> remove once we start relying on it from tests, and that if ultimately, the
> only reason for the pref to exist is for the toggle in the preferences, we
> might as well not have the preference and just make ticking / unticking the
> checkbox call CUI APIs directly. That then removes the need for all the pref
> vs. CUI tracking machinery, and removes potential for bugs where they could
> be out of sync, plus we wouldn't store the same information twice. We would
> keep a searchwidgettracker-like thing to make it a 1-line change to
> add/remove the search bar, or to check whether it's present, but I think
> that would ultimately reduce complexity.

The main reason we need the pref is to keep the option to use it in a simple system add-on or Shield study in order to migrate users to a unified bar. We are going to tread very carefully here and there are already a number of studies in flight, but it looks like the result analysis will come back too late in the game for implementing a patch. We would like to keep the simple option of flipping a pref in release for a still unidentified segment of the user population and give them the modern experience.
FWIW, part of the perf impact here might be the flexible spacers which are being added by CUI, which may cause a layout rejig. Ditto for some late icon loading (cf. bug 1394914, bug 1374224). I expect the fact that there are fewer (high) flex items here would influence the degree to which things resize from those operations, which in turn might mean more stuff gets invalidated, which could potentially have negative perf implications for rendering. But that's just me guessing - maybe Milan can confirm if this is at all plausible?

I don't think that should stop this patch landing, we should instead try to work out how to have flexible spacers in the markup so CUI doesn't need to add them dynamically (and add minimum sizes to icons in the other bugs I mentioned). That can be a followup.

(In reply to Panos Astithas [:past] (56 Regression Engineering Owner) (please ni?) from comment #25)
> (In reply to :Gijs (queue backed up, slow) from comment #21)
> > I think the abstraction through the pref is something that will be harder to
> > remove once we start relying on it from tests, and that if ultimately, the
> > only reason for the pref to exist is for the toggle in the preferences, we
> > might as well not have the preference and just make ticking / unticking the
> > checkbox call CUI APIs directly. That then removes the need for all the pref
> > vs. CUI tracking machinery, and removes potential for bugs where they could
> > be out of sync, plus we wouldn't store the same information twice. We would
> > keep a searchwidgettracker-like thing to make it a 1-line change to
> > add/remove the search bar, or to check whether it's present, but I think
> > that would ultimately reduce complexity.
> 
> The main reason we need the pref is to keep the option to use it in a simple
> system add-on or Shield study in order to migrate users to a unified bar. We
> are going to tread very carefully here and there are already a number of
> studies in flight, but it looks like the result analysis will come back too
> late in the game for implementing a patch. We would like to keep the simple
> option of flipping a pref in release for a still unidentified segment of the
> user population and give them the modern experience.

So I'm a bit confused again now... the pref will be false (search bar not in the UI) by default, right? And then it will be `true` after the CUI saved state loads into the browser. And then we intend to flip it back to false for some users of 57 who upgraded from 56 or earlier, or something?
Assignee

Updated

2 years ago
Blocks: 1396562
Assignee

Updated

2 years ago
Blocks: 1396569
Assignee

Comment 27

2 years ago
(In reply to :Gijs (queue backed up, slow) from comment #26)
> So I'm a bit confused again now... the pref will be false (search bar not in
> the UI) by default, right? And then it will be `true` after the CUI saved
> state loads into the browser. And then we intend to flip it back to false
> for some users of 57 who upgraded from 56 or earlier, or something?

As I understand it, this is the general idea, though Panos might have a broader picture.

It seems to me this mostly provides a fallback mechanism that we can use if necessary. In my opinion we should still try to use the more robust migration steps, system add-ons, or hotfixes, but we might find ourselves in a situation where that might not be possible because of time constraints, and the presence of the preference gives some more confidence around these topics.
(In reply to Panos Astithas [:past] (56 Regression Engineering Owner) (please ni?) from comment #23)
> Milan, do we care at this point about Talos regressions with Quantum Render?
> If we do, can we get somebody from that team to help us understand what is
> the problem here?

We'll take the regressions.
Flags: needinfo?(milan)
Comment hidden (mozreview-request)
Assignee

Comment 30

2 years ago
mozreview-review
Comment on attachment 8903663 [details]
Bug 1387416 - Place the search bar in the customization palette for new profiles.

https://reviewboard.mozilla.org/r/175434/#review181050

::: browser/components/customizableui/test/browser_913972_currentset_overflow.js:27
(Diff revisions 1 - 2)
> +
> +  // Restore the window to its original state.
> +  urlbar.style.removeProperty("min-width");
>    window.resizeTo(originalWindowWidth, window.outerHeight);
>    await waitForCondition(() => !navbar.hasAttribute("overflowing"));

This is still failing, but I'm not sure what's going on:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4082071475d213aaea972c5cebe652d206831584&selectedJob=128316704

If you have any suggestions they're welcome. In the meantime I'll try running the entire folder locally, maybe I could then be able to see the failure.
Paolo is exactly right in comment 27.
Assignee

Comment 32

2 years ago
(In reply to :Paolo Amadini from comment #30)
> This is still failing, but I'm not sure what's going on:

So, I reduced this to minimal steps to reproduce locally. If I run browser_913972_currentset_overflow.js alone, it behaves normally. If I run browser_901207_searchbar_in_panel.js just before it, and in particular the overflow test, then when browser_913972_currentset_overflow.js runs it behaves strangely.

The issue is that when the window is resized back to its normal size, the location bar uses up all the space, the flexible spacers are not added back, and all the controls are overflowed. Weirdly, if introduce a five second pause before the test shrinks the window, and I use this time to resize the window to a smaller size and back manually, then when the test continues it behaves normally again, although there seems to be no difference in the position of the controls. This makes me think there is some broken internal state caused by the sudden jumping resize. Calling CustomizableUI.reset at the end of the previous test does not help.

Gijs, does this ring any bell?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 33

2 years ago
So, it seems that the order things are done in browser_901207_searchbar_in_panel.js breaks the other test because on resize the CustomizableUI code thinks there are still 6 pixels of overflow. My guess is that this is somehow related to the searchbar splitter. I have resolved the issue by hiding the search bar before the second window resize in the first test.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 34

2 years ago
The tests pass locally, I've just started a tryserver build and I'm posting the new version for review in the meantime.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7d9754c5e94499616e0761313641bf88cf4faaf
Comment hidden (mozreview-request)

Comment 36

2 years ago
mozreview-review
Comment on attachment 8903663 [details]
Bug 1387416 - Place the search bar in the customization palette for new profiles.

https://reviewboard.mozilla.org/r/175434/#review181314

Out of interest, why the switch to a 300px width instead of 200?
Attachment #8903663 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Comment 37

2 years ago
(In reply to :Gijs (queue backed up, slow) from comment #36)
> Out of interest, why the switch to a 300px width instead of 200?

It used to be 400 originally. This may still be enough on all platforms, but I reduced it to 300 just in case. I only used the value 200 to see if this helped while debugging the overflow issues.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Whiteboard: [fxsearch] → [fxsearch][small linux-qr tresize regression expected]
Assignee

Comment 40

2 years ago
This is now waiting on the backlog of OS X tests.

There is also a failure of browser_UsageTelemetry_content_aboutHome.js on Windows, but it looks like it's a known issue related to Activity Stream introduced by bug 1394777.

Comment 41

2 years ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0338f82cf70e
Place the search bar in the customization palette for new profiles. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/0338f82cf70e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

2 years ago
Depends on: 1398416

Updated

2 years ago
Depends on: 1398566

Updated

2 years ago
Depends on: 1398567

Comment 44

2 years ago
I have reproduced this bug with Nightly 57.0a1 (2017-08-04) on Windows 8.1, 64 Bit!

This bug's fix is verified on Latest Nightly 57.0a1.

Build ID :  20170910100150
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170906]
Many improvements noticed:

== Change summary for alert #9302 (as of September 07 2017 07:03 UTC) ==

Improvements:

  5%  sessionrestore windows10-64 opt e10s     544.58 -> 516.67
  4%  sessionrestore windows7-32 opt e10s      592.85 -> 567.62
  4%  sessionrestore linux64 opt e10s          492.83 -> 473.08
  4%  tpaint summary windows10-64 opt e10s     274.90 -> 264.50
  3%  sessionrestore linux64 pgo e10s          440.67 -> 428.25
  3%  sessionrestore osx-10-10 opt e10s        683.42 -> 664.42
  3%  ts_paint linux64 opt e10s                873.75 -> 849.67
  2%  ts_paint linux64 pgo e10s                785.21 -> 766.83
  2%  tart summary linux64 opt e10s            6.06 -> 5.93
  2%  tpaint summary windows10-64 pgo e10s     223.55 -> 219.96

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9302
Panos, do we have people with cycles to work on some polish for the "search from the awesomebar" behaviour, per some of the dependent bugs? I'm happy to provide pointers to the relevant code, but I don't have time to address this myself, and I'm aware Paolo is on PTO right now.
Flags: needinfo?(past)
I took a look and prioritized the dependencies. None of them is severe enough for 57, but we will work on them in due course.
Flags: needinfo?(past)

Updated

2 years ago
Depends on: 1398887
Depends on: 1399968
I have reproduced this Bug with Nightly Nightly 57.0a1 (2017-08-04) (64-bit) on Ubuntu 16.04 LTS!

The bug's fix is now verified on latest Nightly!

Build ID :   20170921100141
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170920]
Depends on: 1407295
You need to log in before you can comment on or make changes to this bug.