1174821 - Box-model view code clean-up

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

Some cleanup needed in both the css and js part of this panel, as a prerequisite for all bugs blocking bug 1150496.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1174821 - 1 - eslint cleanup of view.js; r=janx

Bug 1174821 - 1 - eslint cleanup of view.js

Comment 2

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover; r=janx

Bug 1174821 - 2 - Show only 1 box-model region on hover

Comment 3

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files; r=janx

Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files

The layout-view used 2 css files, one in /browser/devtools/layoutview
and the other in /browser/themes.
This makes it unnecessarily confusing for people looking for the right
place to make css changes.
This change re-unit all css rules in one file under /browser/themes
and cleans up the code a bit (better structure, comments, simplifications).

Comment 4

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1174821 - 4 - Refactor layoutview's dimming; r=janx

Bug 1174821 - 4 - Refactor layoutview's dimming

Comment 5

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1174821 - 5 - More eslint cleanup of view.js; r=janx

Bug 1174821 - 5 - More eslint cleanup of view.js

Comment 6

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=572e79947c4e

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #2)
> Created attachment 8622601 [details]
> MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover
> 
> Bug 1174821 - 2 - Show only 1 box-model region on hover
For info, part 2 is about changing the way the box-model of the current node gets highlighted in the page when hovering over the 4 different regions in the layoutview UI. We used to always highlight all 4 regions in the page but just move the guides around the hovered region.
We already discussed in the past about changing that to only highlight the corresponding region, and that's what Chrome does too, so I took the opportunity of this bug to do this.

Comment 8

[Patrick Brosset <:pbro>]

Looks like I caught a permafailing test when I pushed to try yesterday:
> browser/devtools/debugger/test/browser_dbg_WorkerActor.attachThread.js | A promise chain failed to handle a rejection: at chrome://mochitests/content/browser/browser/devtools/debugger/test/head.js:1180 - TypeError: sources is undefined
This seems unrelated.

Comment 9

[Jan Keromnes [:janx]]

Comment on attachment 8622601 [details]
MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover; r=janx

I'm not sure I like this change. I wasn't part of that discussion, but where Chrome highlights just the border of an element separately from the other regions, the new behavior after this patch is to highlight content + padding + border in the same color (all contained regions get highlighted).

If we're going to highlight these regions separately, let's at least match Chrome's behavior in highlighting precisely (without the contained regions).

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Jan Keromnes [:janx] from comment #9)
> Comment on attachment 8622601 [details]
> MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover
> 
> I'm not sure I like this change. I wasn't part of that discussion, but where
> Chrome highlights just the border of an element separately from the other
> regions, the new behavior after this patch is to highlight content + padding
> + border in the same color (all contained regions get highlighted).
> 
> If we're going to highlight these regions separately, let's at least match
> Chrome's behavior in highlighting precisely (without the contained regions).
Now that you mention it, we indeed wanted to do this too. However this requires some changes to the highlighter (right now it cannot do this).

Comment 11

[Jan Keromnes [:janx]]

Comment on attachment 8622602 [details]
MozReview Request: Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files; r=janx

> --- a/browser/themes/shared/devtools/layoutview.css
> +++ b/browser/themes/shared/devtools/layoutview.css
> @@ -6,11 +6,62 @@
>    box-sizing: border-box;
>  }
>  
> +body {
> +  /* The view will grow bigger as the window gets resized, until 400px */
> +  max-width: 400px;

Nit: 400px is very large for the box model view, don't you think? I personally preferred 320px, but I'll let you make this call.

> +#margins {
> +  /* This opacity applies to all of the regions, since they are nested. */

Super-nit: This is the only comment with a period at the end. Please consistently add or omit them.

> +/* Coordinates should be different when the window is small, because we make
> +   the regions smaller then */
> +
> +
> +@media (max-height: 228px) {

Super-nit: Please remove the extra empty line.

Comment 12

[Jan Keromnes [:janx]]

Comment on attachment 8622603 [details]
MozReview Request: Bug 1174821 - 4 - Refactor layoutview's dimming; r=janx

> +  isViewVisibleAndValidNode: function() {

That function name is quite the tongue twister! But maybe shorter names like "isValid" or "hasValidNode" would have been confusing or misleading.

> +    if (isActive) {
> +      this.doc.body.classList.remove("inactive");
> +      this.trackReflows();
> +    } else {
> +      this.doc.body.classList.add("inactive");
> +      this.untrackReflows();
> +    }

Nit: You can factor out the classList lines by using toggle("inactive", !isActive).

Comment 13

[Jan Keromnes [:janx]]

Comment on attachment 8622604 [details]
MozReview Request: Bug 1174821 - 5 - More eslint cleanup of view.js; r=janx

> -const SHARED_L10N = new ViewHelpers.L10N("chrome://browser/locale/devtools/shared.properties");
> +const BUNDLE = "chrome://browser/locale/devtools/shared.properties";
> +const SHARED_L10N = new ViewHelpers.L10N(BUNDLE);

Nit: The usual name for BUNDLE seems to be STRINGS_URI (or something with "STRINGS" in it).

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8622600 [details]
MozReview Request: Bug 1174821 - 1 - eslint cleanup of view.js; r=janx

Bug 1174821 - 1 - eslint cleanup of view.js; r=janx

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8622601 [details]
MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover; r=janx

Bug 1174821 - 2 - Show only 1 box-model region on hover; r=janx

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8622602 [details]
MozReview Request: Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files; r=janx

Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files; r=janx

The layout-view used 2 css files, one in /browser/devtools/layoutview
and the other in /browser/themes.
This makes it unnecessarily confusing for people looking for the right
place to make css changes.
This change re-unit all css rules in one file under /browser/themes
and cleans up the code a bit (better structure, comments, simplifications).

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8622603 [details]
MozReview Request: Bug 1174821 - 4 - Refactor layoutview's dimming; r=janx

Bug 1174821 - 4 - Refactor layoutview's dimming; r=janx

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8622604 [details]
MozReview Request: Bug 1174821 - 5 - More eslint cleanup of view.js; r=janx

Bug 1174821 - 5 - More eslint cleanup of view.js; r=janx

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8622600 [details]
MozReview Request: Bug 1174821 - 1 - eslint cleanup of view.js; r=janx

I don't know why mozreview keeps asking for reviews on patches that have already been R+'d, and I don't know if there's a way for me to avoid this.

Comment 20

[Jan Keromnes [:janx]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #19)
> I don't know why mozreview keeps asking for reviews on patches that have
> already been R+'d, and I don't know if there's a way for me to avoid this.

I think mozreview doesn't care about actual attachment flags. Last time I used it, it didn't r+ the patches when I used "Ship it!", and here I didn't use "Ship it!" so that's probably why it failed to realized that the patches were approved.

Comment 21

[Jan Keromnes [:janx]]

Could you please rebase your patches and re-submit to try, just to be sure? Other try pushes don't seem to have these permafail oranges.

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Keromnes [:janx] from comment #20)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #19)
> > I don't know why mozreview keeps asking for reviews on patches that have
> > already been R+'d, and I don't know if there's a way for me to avoid this.
> 
> I think mozreview doesn't care about actual attachment flags. Last time I
> used it, it didn't r+ the patches when I used "Ship it!", and here I didn't
> use "Ship it!" so that's probably why it failed to realized that the patches
> were approved.

MozReview's behavior has recently changed: it used to only make one BZ attachment for the entire parent review, and you had to "ship it!" on the root review request to see it in BZ.

We now live in a more logical time, where MozReview makes attachments per commit.  When reviewing, you can "ship it!" per commit, and it should appear in BZ for just that commit.

If you upload a future revision, you may need to "re-ship it!" modified commits yourself.  I think you can do that before publishing the updated review draft.

You can always file MozReview bugs to improve it as well.  It is actively improved and worked on.

Comment 23

[Patrick Brosset <:pbro>]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22) 
> You can always file MozReview bugs to improve it as well.  It is actively
> improved and worked on.
Yeah, that's what I did. Thanks for the clarification Ryan.

Comment 24

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2058852061d4

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0a83b62ea755
https://hg.mozilla.org/integration/fx-team/rev/95869bbbeaba
https://hg.mozilla.org/integration/fx-team/rev/588059d28c70
https://hg.mozilla.org/integration/fx-team/rev/22b8d6aa174f
https://hg.mozilla.org/integration/fx-team/rev/721e9f35623f

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a83b62ea755
https://hg.mozilla.org/mozilla-central/rev/95869bbbeaba
https://hg.mozilla.org/mozilla-central/rev/588059d28c70
https://hg.mozilla.org/mozilla-central/rev/22b8d6aa174f
https://hg.mozilla.org/mozilla-central/rev/721e9f35623f