Closed Bug 1451282 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file)

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.
Blocks: 1451283
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)
(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 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+
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
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.
Well, lots of failures locally for some reason, trying on infrastructure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70268c9c68bd9baab501b00a095d832ea68b981a
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
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
https://hg.mozilla.org/mozilla-central/rev/5016bdff6bb5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.