1451282 - Don't use a groupbox for the meta tree in the Page Info window

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect, P1)

Description

[:Paolo Amadini]

The Page Info window uses a collapsible groupbox that simply disappears and leaves empty space. This should be converted to a simple groupbox or just a label, and the feature should be removed since this is the last usage in mozilla-central.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1451282 - Don't use a groupbox for the meta tree in the Page Info window.

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

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8965039 [details]
Bug 1451282 - Don't use a groupbox for the meta tree in the Page Info window.

https://reviewboard.mozilla.org/r/233770/#review239416

Thanks, this is a nice cleanup. I have two suggestions that are mutually exclusive and I don't feel strongly about which one to do (although I slightly prefer the second)

::: browser/base/content/pageinfo/pageInfo.js:513
(Diff revision 1)
> -  else {
> -    var metaTagsCaption = document.getElementById("metaTagsCaption");
> +  var metaTagsCaption = document.getElementById("metaTagsCaption");
> +  var metaTree = document.getElementById("metatree");
> +  if (!length) {
> +    metaTagsCaption.hidden = true;
> +    metaTree.style.visibility = "hidden";

Couldn't we use `metaTree.collapsed = true` and `metaTree.collapsed = false` instead of `metaTree.style.visibility = "hidden";` and `metaTree.style.removeProperty("visibility");`?

::: browser/base/content/pageinfo/pageInfo.xul:145
(Diff revision 1)
>              <textbox readonly="true" id="modifiedtext"/>
>            </row>
>          </rows>
>        </grid>
>        <separator class="thin"/>
> -      <groupbox id="metaTags" flex="1" class="collapsable treebox">
> +      <label control="metatree" id="metaTagsCaption" class="header"/>

If we switched the groupbox to a vbox there would need to be less JS changes (i.e. only change the way the label text gets set).

Comment 3

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Couldn't we use `metaTree.collapsed = true` and `metaTree.collapsed = false`
> instead of `metaTree.style.visibility = "hidden";` and
> `metaTree.style.removeProperty("visibility");`?

This was done to fix the position of the help button when the tree is hidden, I've updated the commit message to mention it.

Comment 4

[:Paolo Amadini]

Comment on attachment 8965039 [details]
Bug 1451282 - Don't use a groupbox for the meta tree in the Page Info window.

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

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8965039 [details]
Bug 1451282 - Don't use a groupbox for the meta tree in the Page Info window.

https://reviewboard.mozilla.org/r/233770/#review239694

Comment 6

[:Paolo Amadini]

I don't think this specific part of the window is covered by tests, but let's see a tryserver build just in case I missed them:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e5b778a9f74d617e034fef321bd46e192d4f375

Comment 7

[:Paolo Amadini]

Comment on attachment 8965039 [details]
Bug 1451282 - Don't use a groupbox for the meta tree in the Page Info window.

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

Comment 8

[:Paolo Amadini]

The browser_all_files_referenced.js test detected one file that is now unused on macOS, I removed it and will land with this change after a local test run.

Comment 9

[:Paolo Amadini]

Well, lots of failures locally for some reason, trying on infrastructure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70268c9c68bd9baab501b00a095d832ea68b981a

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5016bdff6bb5
Don't use a groupbox for the meta tree in the Page Info window. r=bgrins

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5016bdff6bb5