1273958 - Discovery page shows search an tools if navigated to from outside about:addons

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Markus Jaritz [:designakt] (UX)]

Attachment: about-addons-discovery-search-visible.gif

Discovery page shows search an tools if navigated to from outside about:addons, but not if navigated to from within about:addons.
When it should never show those functions on the discovery page.

Steps to reproduce:
go to about:addons
select "Get Add-ons"
close about:addons
open about:addons

Experienced in a new profile of the current Nightly on Windows 10.

attached a screenrecording for reference.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1273958 Fix suppression of search box for disco pane

We added the code to hide the header in bug 1262214 but it went
inside a block that is only executed when we're changing views.
As a result, if we open the add-on manager looking at the discovery
page, the header isn't hidden.  Fix it by making it unconditional.

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

Comment 2

[Robert Helmer [:rhelmer]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

https://reviewboard.mozilla.org/r/54376/#review51218

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc23eccfb28

Comment 4

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/85ce23415ab0 for https://treeherder.mozilla.org/logviewer.html#?job_id=28489436&repo=mozilla-inbound

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

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

Comment 6

[Andrew Swan [:aswan]]

Now with fixes for affected tests.  There were several tests that tried to initiate searches from the discovery pane which obviously doesn't work when there's no search box in the discovery pane.  Most of the edits were straightforward, the one in browser_searching.js was slightly more involved since the test appears to want to verify some behavior at the discovery pane and then launch a new search.  I added an extra navigation to the extension list between those steps.  That merits a close look in review though in case I misunderstood what the test is meant to do.

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

whoops, rhelmer is out this week, i think that just leaves you kris

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

https://reviewboard.mozilla.org/r/54376/#review52552

Looks OK to me, but you've got some failing tests on try. r=me assuming those are trivial test fixes.

Comment 9

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #8)
> Looks OK to me, but you've got some failing tests on try. r=me assuming
> those are trivial test fixes.

I think these are all known intermittent failures?

Comment 10

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #9)
> (In reply to Kris Maglione [:kmag] from comment #8)
> > Looks OK to me, but you've got some failing tests on try. r=me assuming
> > those are trivial test fixes.
> 
> I think these are all known intermittent failures?

I don't think so. They happened 4 times in one test run, and are related to the code that you changed, so they're almost certainly perma-orange now.

Comment 11

[Andrew Swan [:aswan]]

Ugh this is turning into a saga.  I think the only remaining failure here is that browser_details.js is now failing consistently, but only in the "test-window" variant.  I think the problem is that starting out with the header hidden changes the computation of the initial window size, which causes the window to not be large enough to display the full details for an add-on.  Since the "disable" button is at the bottom of the detail pane, it is no longer on the screen by default so trying to click it here (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_details.js#250) fails.

In the process of banging my head against the wall on this, I tried to do some experimentation with a live browser and I found many ways in the UI to open the add-ons manager in a tab but no way short of calling `openDialog()` from the browser toolbox to open it in a dedicated top-level window as this test does.  So before stressing about this any further, is there a good reason to keep running the test-window variants of the add-ons manager browser tests?  If they're a relic of something that's no longer supported, not running them would save us some test cycles and I think let me close out this bug easily.

Comment 12

[Dave Townsend [:mossop]]

(In reply to Andrew Swan [:aswan] from comment #11)
> Ugh this is turning into a saga.  I think the only remaining failure here is
> that browser_details.js is now failing consistently, but only in the
> "test-window" variant.  I think the problem is that starting out with the
> header hidden changes the computation of the initial window size, which
> causes the window to not be large enough to display the full details for an
> add-on.  Since the "disable" button is at the bottom of the detail pane, it
> is no longer on the screen by default so trying to click it here
> (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/browser/browser_details.js#250) fails.
> 
> In the process of banging my head against the wall on this, I tried to do
> some experimentation with a live browser and I found many ways in the UI to
> open the add-ons manager in a tab but no way short of calling `openDialog()`
> from the browser toolbox to open it in a dedicated top-level window as this
> test does.  So before stressing about this any further, is there a good
> reason to keep running the test-window variants of the add-ons manager
> browser tests?  If they're a relic of something that's no longer supported,
> not running them would save us some test cycles and I think let me close out
> this bug easily.

As part of toolkit the add-ons manager is designed to be usable in any application that runs on our platform including those that don't have the concept of tabs so we support opening the manager in a stand-alone window. Thunderbird and Seamonkey used to use this but I think they have switched to tabs now anyway. It's possible that this is no longer needed by any app we care about and even if it was we shouldn't spend undue time on fixing this, but I'm wary of dropping the tests wholesale for the sake of one failing test. Can we add a window size to the openDialog call that is large enough to fix this? It probably makes sense to use a fixed size in tests anyway. If not then let's just only run that test in the normal tabbed mode and move on.

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

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

Comment 14

[Andrew Swan [:aswan]]

I took a different approach and restructured the page layout so the disco pane is separate from the panes that include the header, those panes are all in a shared vbox with a header and a deck.  This restores all the geometry calculations to the way they worked before, and is arguably "more correct".  It does take the patch from fewer than 10 lines to more than 500 lines, but a bunch of that is whitespace in extensions.xul.

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

This is basically brand new, review should start over again.

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

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

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

https://reviewboard.mozilla.org/r/54376/#review54604

I like this much better. Thanks! Just a few nits to address.

::: toolkit/mozapps/extensions/content/extensions.js:756
(Diff revision 4)
>      this.loadViewInternal(aViewId, null, state);
>      this.initialViewSelected = true;
>      notifyInitialized();
>    },
>  
> +  findCurrentPanel: function() {

Let's just make this a getter.

We might as well add a setter for the same property so we can just assign the panel we want to select and have it Just Work.

::: toolkit/mozapps/extensions/content/extensions.js:791
(Diff revision 4)
>      gCategories.select(aViewId, aPreviousView);
>  
>      this.currentViewId = aViewId;
>      this.currentViewObj = viewObj;
>  
> -    this.viewPort.selectedPanel = this.currentViewObj.node;
> +    // XXX discover-view is its own panel, everything else is a child of

No XXX comments please.

::: toolkit/mozapps/extensions/content/extensions.js:796
(Diff revision 4)
> -    this.viewPort.selectedPanel = this.currentViewObj.node;
> -    this.viewPort.selectedPanel.setAttribute("loading", "true");
> +    // XXX discover-view is its own panel, everything else is a child of
> +    // headered-views.  maybe this should be more general (ie, check if
> +    // currentViewObj is a child of headered-views instead of hard
> +    // coded check for discover)
> +    let panel;
> +    if (view.type == "discover") {

How about `if (node.parentNode === this.headeredViewsDeck)` ?

::: toolkit/mozapps/extensions/content/extensions.xul:343
(Diff revision 4)
>  <span class="indent">>></span>              <hbox pack="center">
>  <span class="indent">>></span>                <label id="search-allresults-link" class="text-link"/>
>  <span class="indent">>></span>              </hbox>
>  <span class="indent">>></span>            </richlistbox>
>  <span class="indent">>></span>          </vbox>
> -
> +  

Nit: remove whitespace.

::: toolkit/mozapps/extensions/content/extensions.xul:483
(Diff revision 4)
>  <span class="indent">>></span>                      label="&updates.updateSelected.label;"
>  <span class="indent">>></span>                      tooltiptext="&updates.updateSelected.tooltip;"/>
>  <span class="indent">>></span>            </hbox>
>  <span class="indent">>></span>            <richlistbox id="updates-list" class="list" flex="1"/>
>  <span class="indent">>></span>          </vbox>
> -
> +  

Remove whitespace.

::: toolkit/mozapps/extensions/test/browser/browser_bug562797.js:130
(Diff revision 4)
>  
>  function is_in_list(aManager, view, canGoBack, canGoForward) {
>    var doc = aManager.document;
>  
>    is(doc.getElementById("categories").selectedItem.value, view, "Should be on the right category");
> -  is(doc.getElementById("view-port").selectedPanel.id, "list-view", "Should be on the right view");
> +  is(get_current_view(aManager).id, "list-view", "Should be on the right view");

`aManager.gViewController.currentPanel.id`?

::: toolkit/mozapps/extensions/test/browser/head.js:293
(Diff revision 4)
> +  if (view.id == "headered-views") {
> +    return aManager.document.getElementById("headered-views-content").selectedPanel;
> +  } else {
> +    return view;
> +  }
> +  

Nit: empty line with whitespace.

Comment 18

[Kris Maglione [:kmag]]

[Tracking Requested - why for this release]:

The UI change that this bug affects landed in 48 (bug 1262214), which is meant to be a major release milestone for extensions in Firefox. This bug leads to inconsistent behavior, and a user experience that feels much less solid than I'd like to see in this release.

Comment 19

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/54376/#review54604

> `aManager.gViewController.currentPanel.id`?

I feel ambivalent about this one, that would mean that we're trusting the currentPanel (actually currentView) getter rather than testing it, but of course the alternative is that we end up just coyping the implementation of it into get_current_view()...

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54376/diff/4-5/

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

https://reviewboard.mozilla.org/r/54376/#review54828

Excellent. Thanks!

::: toolkit/mozapps/extensions/content/extensions.js:768
(Diff revisions 4 - 5)
>  
> +  set displayedView(view) {
> +    let node = view.node;
> +    if (node.parentNode == this.headeredViewsDeck) {
> +      this.viewPort.selectedPanel = this.headeredViews;
> +      this.headeredViewsDeck.selectedPanel = node;

We should probably do this before we set `viewPort.selectedPanel`. Admittedly, it shouldn't make much difference unless we somehow trigger a reflow in between the two changes.

Comment 22

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/54376/#review54604

> I feel ambivalent about this one, that would mean that we're trusting the currentPanel (actually currentView) getter rather than testing it, but of course the alternative is that we end up just coyping the implementation of it into get_current_view()...

Can we implement the lookup in `get_current_view`, and check that it concurs with `aManager.gViewController.currentPanel` instead, then?

Comment 23

[Andrew Swan [:aswan]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54376/diff/5-6/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0d26f56bffd
Fix suppression of search box for disco pane r=kmag,rhelmer

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a feature we're emphasizing in 48. Can you fill out an uplift approval request? Thanks. It might be good to verify this before uplift to beta

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Andrew, could you fill the uplift request?

Comment 28

[Andrew Swan [:aswan]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

Approval Request Comment
[Feature/regressing bug #]:
1273958

[User impact if declined]:
Awkward navigation -- within the discovery pane (the pane displayed when "Get Add-ons" is selected in the left panel of about:addons), the header that includes the search bar is sometimes displayed and sometimes not displayed.

[Describe test coverage new/current, TreeHerder]:
There are no automated tests covering the specific behavior in this bug.  But the changes here do impact a number of other tests, I'll start separate try runs against beta and aurora to verify that those tests also continue to pass on beta and aurora.

[Risks and why]: 
This patch tweaks the structure of the XUL for about:addons, it is possible that the changes could introduce a regression that is undetected by automated tests.  As for why, this work was originally done for the discovery pane redesign project that is scheduled to go live in 48.

[String/UUID change made/needed]:
none

Comment 29

[Gerry Chang [:gchang]]

Comment on attachment 8755059 [details]
Bug 1273958 Fix suppression of search box for disco pane

This patches fix suppression of search box for discovery pane. Take it in 48 beta 4 and aurora.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b9b86015cdc

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b9b86015cdc

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f7521f8f54ab

Comment 33

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I managed to reproduce this bug using Fx 49.0a1, build ID:20160523030225 on Windows 10 x64.
Confirmed the fix on Fx 48.0b4, build ID:20160627144420 and Fx 49.0a2, build ID:20160628004053 and Fx 50.0a1 build ID:20160629030209.