1368709 - CSS Shapes Highlighter: handle zoom and geometry-box options

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Mike Park [:mparkms]]

In bug 1282716, we create a static CSS shapes highlighter.

This highlighter needs to be able to handle different zoom levels, as well as different bounding boxes based on the geometry-box option for clip-path/shape-outside. This will change the reference box that the shape is drawn in.

Comment 1

[Mike Park [:mparkms]]

Attachment: Bug 1368709 - Handle zoom and geometry-box options for CSS shapes highlighter.

Also convert markers to SVG path to better handle zoom

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

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8872719 [details]
Bug 1368709 - Handle zoom and geometry-box options for CSS shapes highlighter.

https://reviewboard.mozilla.org/r/144252/#review148946

I tested this locally, it's awesome! It just works really nicely, even when changing CSS dynamically, resizing the window, scrolling, zooming! It seems pretty robust so far.
I also like how the code reads very easily. Good job on this.
Could you ask Matteo (:zer0 on bugzilla) to give feedback about this patch? These incremental changes are starting to go into finer and finer details that he happens to know a lot about. So even though I'm more than happy to R+ this, I'd like it if he could have a second pair of expert eyes on this.

::: commit-message-286f7:2
(Diff revision 1)
> +Bug 1368709 - Handle zoom and geometry-box options for CSS shapes highlighter. r=pbro
> +Also convert markers to SVG path to better handle zoom

nit: We usually insert an empty line between the first "Bug xxx - description; reviewer" line and the subsequent longer description lines.
I don't think this will have an impact on how tools use this message though, so no big deal, but it's more consistent with how we usually do this.

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_03.js:35
(Diff revision 1)
> +  let expectedStyle = `top:${top}px;left:${left}px;width:${width}px;height:${height}px;`;
> +
> +  for (let zoom of TEST_LEVELS) {
> +    info(`Setting zoom level to ${zoom}.`);
> +    yield testActor.zoomPageTo(zoom, highlighterFront.actorID);
> +    info("zoomed");

This `info` is not very useful.

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_03.js:39
(Diff revision 1)
> +    yield testActor.zoomPageTo(zoom, highlighterFront.actorID);
> +    info("zoomed");
> +    let style = yield testActor.getHighlighterNodeAttribute(
> +      "shapes-shape-container", "style", highlighterFront);
> +
> +    is(style, expectedStyle, `Highlighter has correct quads at zoom level ${zoom}`);

This test lacks a bit of comments I think. Here we change the zoom level, but every time we just check that the coordinates of the container are the same.
A comment would be nice to explain why we want them to be the same all the time.

::: devtools/client/inspector/test/doc_inspector_highlighter_cssshapes.html:43
(Diff revision 1)
> +  .svg {
> +    width: 800px;
> +    height: 800px;
> +  }
> +  #rect {
> +    clip-path: polygon(0 0,
> +                       100px 50%,
> +                       200px 0,
> +                       300px 50%,
> +                       400px 0,
> +                       500px 50%,
> +                       600px 0,
> +                       700px 50%,
> +                       800px 0,
> +                       90% 100%,
> +                       50% 60%,
> +                       10% 100%);
> +    stroke: red;
> +    stroke-width: 20px;
> +    fill: blue;

Why was this svg element added to the test page? I don't see it being used in the new test you added.

Update: I see why you added this now, since there's special handling in the code if the element having the clip-path is an SVG element. In that case, please it but update the test to also test this.

::: devtools/server/actors/highlighters.css:614
(Diff revision 1)
> -  shape-rendering: crispEdges;
> +  shape-rendering: geometricPrecision;
>    vector-effect: non-scaling-stroke;
>  }
> +
> +:-moz-native-anonymous .shapes-markers {
> +  fill: #000;

Could you add a `--highlighter-marker-color: #000` near the top of this file, where other variables are defined? Let's try and have as few colors hard coded in this file as we can.

::: devtools/server/actors/highlighters/shapes.js:62
(Diff revision 1)
>          "preserveAspectRatio": "none"
>        },
>        prefix: this.ID_CLASS_PREFIX
>      });
>  
> -    // We also need a separate element to draw the shapes' points markers. We can't use
> +    let markersSvg = createSVGNode(this.win, {

Now that we're handling markers in SVG and adapting their sizes and aspect ratio dynamically based on the current zoom and shape's ratio, do we still need to add them into another SVG element?
It looks to me like they could simply go into the first SVG element instead, but maybe I'm missing something.

If we do need a 2nd SVG, then please add a comment here about why.

::: devtools/server/actors/highlighters/shapes.js:598
(Diff revision 1)
> +  // 100px is the base size of markers-container. In order to make the markers'
> +  // size scale properly, we must adjust the radius based on zoom and the width/height of
> +  // the element being highlighted, then calculate a radius for both x/y axes based
> +  // on the aspect ratio of the element.

Thanks for adding this comment, this is very useful because that part is a bit tricky. People might wonder why we care about size and aspect ratio here.
It might be good to make the comment even a bit longer and say that we use a viewBox of 100 x 100 so that it's easy to position things on a relative scale, but that it makes it hard to create circles, etc...

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8872719 [details]
Bug 1368709 - Handle zoom and geometry-box options for CSS shapes highlighter.

See my previous comment, I'd like Matteo to take a quick look at that too.

Comment 4

[Patrick Brosset <:pbro>]

DevTools bug triage (filter on CLIMBING SHOES).

Comment 5

[Mike Park [:mparkms]]

Comment on attachment 8872719 [details]
Bug 1368709 - Handle zoom and geometry-box options for CSS shapes highlighter.

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

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Patrick Brosset <:pbro> from comment #3)
> Comment on attachment 8872719 [details]
> Bug 1368709 - Handle zoom and geometry-box options for CSS shapes
> highlighter.
> 
> See my previous comment, I'd like Matteo to take a quick look at that too.

As mentioned to Mike, I'm going to take a look today.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: css-shape.mov

For the zoom and geometry looks good to me. There are few nits but not related to this patch, more on the highlighter in general. There is one think it concern me though; during the scrolling the highlighter is disappearing (see the attached video). I believe this is related to the fact that the highlighter's content is covering only the viewport with percentage value, plus maybe the async scrolling might be involved.
If we're not planning to display any information on this highlighter (e.g. infobars) maybe resizing the highlighter-content with the same value of the SVG it might fix the issue. Otherwise, we need to handle that as we have done in other highlighters (even if it's not optimal), where we cover the entire document's area.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97bfcea2052c
Handle zoom and geometry-box options for CSS shapes highlighter. r=pbro

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/97bfcea2052c