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

RESOLVED FIXED in Firefox 61

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

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

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

a year ago
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.
Assignee

Updated

a year ago
Blocks: 1451283
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
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).
Attachment #8965039 - Flags: review?(bgrinstead)
Assignee

Comment 3

a year ago
(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 hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
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
Attachment #8965039 - Flags: review?(bgrinstead) → review+
Assignee

Comment 6

a year ago
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 hidden (mozreview-request)
Assignee

Comment 8

a year ago
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.
Assignee

Comment 9

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

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

Comment 10

a year ago
Pushed by paolo.mozmail@amadzone.org:
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
Assignee

Updated

a year ago
Summary: Stop using groupboxes and remove support for the "collapsable" class → Don't use a groupbox for the meta tree in the Page Info window

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5016bdff6bb5
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.