1303158 - Grid Inspector should handle browser zoom

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

The highlighter doesn't handle browser zoom:
  - first of all, the lines get bigger as you zoom in, or thinner as you zoom out, making them hard to see
  - secondly, the highlighter position is wrong. See this screenshot: https://dl.dropboxusercontent.com/u/714210/grid-highlighter-zoom-offset.png

Comment 1

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

Attachment: Bug 1303158 - handle browser zoom and density display in CssGridHighlighter;

That also fixed bug 1303156, since handling better the device pixel ratio makes
also the lines crisp.

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

Comment 3

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

Comment on attachment 8845621 [details]
Bug 1303158 - handle browser zoom and density display in CssGridHighlighter;

Notice that this patch is based on top of bug 1343217.

As we discussed over IRC, I replaced the `WeakMap` with a `Map` since at the current state and due to the bug 1342051, it won't work as intended initially; for multiple highlighters.

It also seems that it's unlikely we're going to have multiple highlighters since the memory limitation for the <canvas> (see Bug 1343217).

In any case, if we decided to go for multiple highlighters, we have to keep a refcount to dispose properly everything, so we will take care of that during the refactoring API of highlighters (for multiple nodes, too).

Comment 4

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

I was working on a bigger refactoring to handling properly density display, browser zoom, crisp lines and transformation; but I decided to land at least this refactoring separately since now with bug 1345434 (creating a virtual canvas) it will takes more time.

Plus, until bug 1345917 and bug 1151421 are not fixed, is not really possible have a perfect handling on sub pixels; that at least will works as close as possible.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8845621 [details]
Bug 1303158 - handle browser zoom and density display in CssGridHighlighter;

https://reviewboard.mozilla.org/r/118752/#review121240

::: devtools/server/actors/highlighters/css-grid.js:53
(Diff revision 1)
>  const GRID_GAP_PATTERN_HEIGHT = 14;
>  const GRID_GAP_PATTERN_LINE_DASH = [5, 3];
>  const GRID_GAP_ALPHA = 0.5;
>  
>  /**
> - * Cached used by `CssGridHighlighter.getGridGapPattern`.
> + * Cached used by `CssGridHighlighter.getGridGapPatternFor`.

s/getGridGapPatternFor/getGridGapPattern

::: devtools/server/actors/highlighters/css-grid.js:361
(Diff revision 1)
>    get color() {
>      return this.options.color || DEFAULT_GRID_COLOR;
>    },
>  
>    /**
> -   * Gets the grid gap pattern used to render the gap regions.
> +   * Gets the grid gap pattern used to render the gap regions, based on the device

Remove the comma before "based on"

::: devtools/server/actors/highlighters/css-grid.js:364
(Diff revision 1)
>  
>    /**
> -   * Gets the grid gap pattern used to render the gap regions.
> +   * Gets the grid gap pattern used to render the gap regions, based on the device
> +   * pixel ratio given.
>     *
> +   * @param {Number} ratio

s/ratio/devicePixelRatio

::: devtools/server/actors/highlighters/css-grid.js:371
(Diff revision 1)
>     * @param  {Object} dimension
> -   *         Refers to the WeakMap key for the grid dimension type which is either the
> -   *         constant COLUMN or ROW.
> +   *         Refers to the Map key for the grid dimension type which is either the
> +   *         constant COLUMNS or ROWS.
>     * @return {CanvasPattern} grid gap pattern.
>     */
> -  getGridGapPattern(dimension) {
> +  getGridGapPatternFor(ratio, dimension) {

s/getGridGapPatternFor/getGridGapPattern

Using For is a bit unnatural for getters since we know we are getting something for something in that makes sense. Better just to be concise for function naming

::: devtools/server/actors/highlighters/css-grid.js:372
(Diff revision 1)
> -   *         Refers to the WeakMap key for the grid dimension type which is either the
> -   *         constant COLUMN or ROW.
> +   *         Refers to the Map key for the grid dimension type which is either the
> +   *         constant COLUMNS or ROWS.
>     * @return {CanvasPattern} grid gap pattern.
>     */
> -  getGridGapPattern(dimension) {
> -    if (gCachedGridPattern.has(dimension)) {
> +  getGridGapPatternFor(ratio, dimension) {
> +    let gridPattern = null;

s/gridPattern/gridPatternMap

::: devtools/server/actors/highlighters/css-grid.js:917
(Diff revision 1)
> +    startPos = Math.round(startPos * ratio);
> +    endPos = Math.round(endPos * ratio);
> +    breadth = Math.round(breadth * ratio);
> +
>      this.ctx.save();
>  

Remove this blank line to group the this.ctx calls.

::: devtools/server/actors/highlighters/css-grid.js:919
(Diff revision 1)
> +    breadth = Math.round(breadth * ratio);
> +
>      this.ctx.save();
>  
> +    this.ctx.fillStyle = this.getGridGapPatternFor(ratio, dimensionType);
>      if (dimensionType === COLUMNS) {

Add a new line before the if statement to separate the blocks.

Comment 6

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

Comment on attachment 8845621 [details]
Bug 1303158 - handle browser zoom and density display in CssGridHighlighter;

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7602bb3a528a
handle browser zoom and density display in CssGridHighlighter; r=gl

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7602bb3a528a

Comment 9

[Bogdan Maris, Desktop Test Engineering]

Attachment: Screenshot showing the issue

I managed to reproduce this issue on old Nightly from 2017-03-12 on Windows 10 X64.

I partial verified this fix on 55.0b12 using Windows 10, Ubuntu 14.04 LTS and macOS 10.12. 
- the lines don't get bigger as you zoom in, or thinner as you zoom out;
but 
- the highlighter position is still wrong when you zoom in and out a few times. (see the attached screenshot)
Could this be related to bug 1342929?

Comment 10

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

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #9)

> - the highlighter position is still wrong when you zoom in and out a few
> times. (see the attached screenshot)
> Could this be related to bug 1342929?

The page also scrolled? Or the position is adjusted once scrolled?
Depends by the answer it could be one bug or another (his bug, instead, was intended to handle the zoom in the grid inspector, so it's considered fixed)

Comment 11

[Jen Simmons [:jensimmons]]

The line numbers stay the same no matter how much soon is applied. I truly believe the line numbers should get bigger or smaller with the zoom, allowing people to use zoom in order to see the numbers better.

For many users, the default line number size is too small. They need a way to increase it, just the way they'd increase text size with zoom.

Any time someone is making a demo onstage or in a video, or taking screenshots for a magazine or blog article, it's very common to zoom into the web page to make important things bigger and therefore more clear. Not allowing the numbers to zoom will make it much harder for people to do demos of our Grid Inspector.

Currently, (as of Sept 13 in Nightly), this is what happens: https://monosnap.com/file/x0MQqXrNe3bg4KsHEsgDa6mT6PW88A.png

Comment 12

[Bogdan Maris, Desktop Test Engineering]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #10)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #9)
> 
> > - the highlighter position is still wrong when you zoom in and out a few
> > times. (see the attached screenshot)
> > Could this be related to bug 1342929?
> 
> The page also scrolled? Or the position is adjusted once scrolled?
> Depends by the answer it could be one bug or another (his bug, instead, was
> intended to handle the zoom in the grid inspector, so it's considered fixed)

To answer you the first question. Yes, the page scrolls too. It scrolls even when you use the zoom from Hamburger Menu. 
The distance between the grid's lines and the highlighter is not that big now but it's still there. The strange part is that there seems to be an inconstancy of where the lines are positioned.
For more details you can see this link https://docs.google.com/document/d/10upTKY-uxVgkU4kakZIY2CEKXPvzu-72Nvkk3X3dSGY/edit

Comment 13

[Daniel Cicas [:dcicas], Release QA]

Hello,

I just checked and it seems that this issue still occurs on the latest Nightly build 2018-12-21 on Win 10 x32 and Ubuntu 18.04 x64 and MacOS 10.13. 

Are there any  plans on fixing this issue in the future?

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Daniel Cicas [:dcicas], Release QA from comment #13)

Hello,

I just checked and it seems that this issue still occurs on the latest
Nightly build 2018-12-21 on Win 10 x32 and Ubuntu 18.04 x64 and MacOS 10.13.

Are there any plans on fixing this issue in the future?

It seems to be working for me. Can you describe the issue? Perhaps filing a new bug as well.

Comment 15

[Daniel Cicas [:dcicas], Release QA]

Attachment: screenshot of the issue on fx 65.0b12

Using Fx 65.0b12 BuildID: 20190117232427 and visiting the website https://labs.jensimmons.com/2017/01-010.html if I zoom in, I can see the lines from the grid overlap the elements.

Comment 16

[Daniel Cicas [:dcicas], Release QA]

Hello Gabriel,

Seeing as this is a really old bug, and it is halfway fixed (the overlap between the grid and the actual element is much smaller now, but the overlap is still slightly visible) should I open a new bug?

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Daniel Cicas [:dcicas], Release QA from comment #16)

Hello Gabriel,

Seeing as this is a really old bug, and it is halfway fixed (the overlap between the grid and the actual element is much smaller now, but the overlap is still slightly visible) should I open a new bug?

Yes, let's open a new bug for this.

Comment 18

[Daniel Cicas [:dcicas], Release QA]

Hello,

I removed the qe-verify+ flag, because I created bug 1533353 for this issue, where the discussion will continue.