1144575 - Investigate how to make the rulers highlighter work on infinite size pages

Status: NEW

(DevTools :: Inspector, defect, P3)

Description

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

The current approach of rulers highlighter is using a SVG with finite size, so documents really huge – e.g. currently more than 10000x15000 – won't have the rulers until the end of the document.

If we want to make them infinite, we have to choice: 
1. create and update the DOM every time the user scroll / resize the window, or the document changes its size (scripts adding dynamic content)
2. using canvas, updating the size when the view port change, and redraw the rulers (see: https://dl.dropboxusercontent.com/u/5374957/highlighters/rules-3.html)

Both of them have costs in term of performance, compared to the current version – we should investigate if we want to implement this, if it makes sense, and in case if we have a better / fast approach for that.

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I'm wondering if we shouldn't just pick a reasonably large default size and then expose a size preference for outlandishly large use cases.

Comment 2

[Potch [:potch]]

We found an outlandishly large use case!

http://s.codepen.io/rachelnabors/fullpage/33183f854614cd9394fea3145d9d0b42?

Also, here's a non-outlandish example of more than 15k pixels of content:

http://www.anandtech.com/show/9662/iphone-6s-and-iphone-6s-plus-preliminary-results

We might try to showcase the rulers in an upcoming demo, but that demo is scroll-heavy. Fixing this issue sort-of blocks that aspect of the demo.

Comment 3

[Joe Walker [:jwalker]]

If you want an easy fix s/15000/20000/ and it starts to look like an easter egg that references https://en.wikipedia.org/wiki/Twenty_Thousand_Leagues_Under_the_Sea

Comment 4

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

I made a prototype:

https://dl.dropboxusercontent.com/u/5374957/highlighters/rules-canvas.html

(use https://dl.dropboxusercontent.com/u/5374957/highlighters/rules-canvas.html?2 if you're on retina)

It's deal with all the issue we have currently in rulers; and also it handle quite nicely the scaling (trying to zoom in and zoom out till the max). 

The only issue is, it uses <canvas> and currently we can't use <canvas> in highlighters. I've a hack that permit that with user interaction, made only for prototyping and testing purpose, but I think it's promising. I'm working on supporting the <canvas> in the proper way, so that we can have such code: https://gist.github.com/ZER0/9607c493f912007b14a5 where the `CanvasHighlighter` encapsulate all the markup, common events, etc, and the developer can focus just on the highlighter, and blit on the canvas.

If you're trying on Nightly the prototype, it could be laggy. That's due bug 1212020, unfortunately affects all our highlighters. It also seems that is not going to be "fixed" (see the bug for additional info), but at least it should be reduced, hope this would be sufficient.

Comment 5

[Patrick Brosset <:pbro>]

(In reply to Matteo Ferretti [:matteo][:zer0] from comment #4)
> If you're trying on Nightly the prototype, it could be laggy. That's due bug
> 1212020, unfortunately affects all our highlighters. It also seems that is
> not going to be "fixed" (see the bug for additional info), but at least it
> should be reduced, hope this would be sufficient.
This prototype looks pretty nice. I have noticed the lagging on nightly but I don't think it's too bad.

Comment 6

[Potch [:potch]]

We've decided not to use the Rulers in the aforementioned demo, so the urgency is less, but long webpages are still long.

Comment 7

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

I've already a patch for that, but it needs bug 1212477 to be fixed. I already mentioned that to ehsan, we'll work on it.

Comment 8

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

Attachment: Investigate how to make the rulers highlighter work on infinite size pages

Comment 9

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

Comment on attachment 8685458 [details] [diff] [review]
Investigate how to make the rulers highlighter work on infinite size pages

This patch is made on top of Bug 1212477's one.

I'm still not sure how we should deal with the tests, here, so I think we should discuss here the alternatives; otherwise before landing this patch we have to remove the old tests.

This implementation also fixes bug 1200114 and bug 1144587.

Comment 10

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

Attachment: Investigate how to make the rulers highlighter work on infinite size pages

Comment 11

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

Comment on attachment 8685477 [details] [diff] [review]
Investigate how to make the rulers highlighter work on infinite size pages

Sorry, I missed a commit in the last patch, this should be the right one.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8685477 [details] [diff] [review]
Investigate how to make the rulers highlighter work on infinite size pages

Review of attachment 8685477 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, I like the code a lot. Thanks for giving me the opportunity to learn about Symbols too.
One major blocker though: on my windows 10, I see no text at all. The rulers are visible, with the graduations, but no labels appear.

About tests: I think you could write at least a unit test-like test for the CanvasHighlighter class.
This would be a simple test that instantiates the class, passing it an instance of highlighterEnv that you would construct yourself, with a test window. You would then test that viewportchange events are emitted when needed, and that update events are emitted when needed too, giving you a reference to a canvas context you can use.
The test could also ensure that the update only starts once show is called, and stops when hide is called.

Also, I believe you could also write a unit test-like test for the RulersHighlighter class. This one would verify the textStep and graduationStep calculations.

::: devtools/server/actors/highlighters/canvas.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +

Please add a comment here, at the very beginning, that explains what this new module is supposed to contain.

@@ +21,5 @@
> +const contexts = new WeakMap();
> +
> +/**
> + * The CanvasHighlighter is a class used for canvas-based highlighters;
> + * it a single canvas that cover the entire page

Phrasing is wrong, maybe this instead:
"It uses a single <canvas> element that covers the entire page"
Also, please explain that this is a parent class intended to be extended, and explain what sub-classes are expected to implement.

@@ +23,5 @@
> +/**
> + * The CanvasHighlighter is a class used for canvas-based highlighters;
> + * it a single canvas that cover the entire page
> + * vertical rules on the page, along the top and left edges, with pixel
> + * graduations, useful for users to quickly check distances

These last 2 lines seem like copy/pasted from the Rulers highlighter class comment, please remove them.

@@ +33,5 @@
> +
> +  let { pageListenerTarget } = this.env;
> +  pageListenerTarget.addEventListener("pagehide", this[_eventHandler]);
> +
> +  setListeners(this, Object.getPrototypeOf(this));

Wow, I had no idea this existed. This could save us a lot of code right? I have a feeling we generally don't know enough about the addon SDK. I think it would be nice creating a 5 minutes thursday tech talk about the most useful helpers. Would you like to do this?
Also, why not use this to also register the pagehide event?

@@ +37,5 @@
> +  setListeners(this, Object.getPrototypeOf(this));
> +
> +  this.markup = new CanvasFrameAnonymousContentHelper(highlighterEnv, () => {
> +    let { window } = this.env;
> +    let prefix = this.ID_CLASS_PREFIX;

You should probably define this with a default value somewhere in this class, and at least mention it in the jsdoc comment that it's required from subclass to define it.

@@ +64,5 @@
> +  });
> +}
> +
> +CanvasHighlighter.prototype = {
> +  typeName: "CanvasHighlighter",

The typeName property isn't used here, and we want to enforce that each and every highlighter defines its own unique one. So I would rather not define it here so it fails when people forget to choose a type for their new highlighter class.

@@ +76,5 @@
> +      }
> +    }
> +  },
> +
> +  [_update]() {

Please add a jsdoc comment block for this method that explains that it gets called in a rAF loop, etc.

@@ +95,5 @@
> +        width: innerWidth,
> +        height: innerHeight,
> +        pixelRatio: devicePixelRatio
> +      });
> +      this[_viewport] = viewport;

I found it a little bit confusing that you're accessing the viewport via both the getter and the symbol. I was originally going to say that you should remove the getter and just use the symbol everywhere, but since you need to getter in rulers.js, then please just use the getter everywhere instead and add a setter.

@@ +99,5 @@
> +      this[_viewport] = viewport;
> +
> +      let canvasWidth = Math.round(innerWidth * devicePixelRatio);
> +      let canvasHeight = Math.round(innerHeight * devicePixelRatio);
> +      this.markup.setAttributeForElement(root, "width", canvasWidth);

The code doesn't have many comments. It's pretty clear and easy to read but still, I think comments would help. I think the most important reason for them is that the CanvasHelper thing API is non-standard and not many people know about it (only the 2 of us I think), so even if the code reads easily, it's hard to know what things do without going and checking other files and wikis.
For example I would add a comment before this block saying that this accesses the anonymous <canvas> element in the CanvasFrame to set its size according to the viewport.
But more generally, I think this file could use some more comments here and there (at least jsdoc for each method).

@@ +157,5 @@
> +    let context2d = this.markup.getCanvasContext(prefix + "root", "2d");
> +    contexts.set(this, context2d);
> +    context2d.mozImageSmoothingEnabled = false;
> +
> +    this.markup.removeAttributeForElement(this.ID_CLASS_PREFIX + "root",

You've created a prefix local variable, you could use it here too.

::: devtools/server/actors/highlighters/rulers.js
@@ +30,5 @@
> +  // They could change based on zoom, see `viewportchange` event.
> +  graduationStep: DEFAULT_GRADUATION_STEP,
> +  textStep: DEFAULT_TEXT_STEP,
> +
> +  onViewportChange() {

Please add a jsdoc comment that explains that this is auto-registered thanks to the CanvasHighlighter class and that it is called everytime the width, height or zoom changes.

@@ +49,5 @@
> +      this.graduationStep = DEFAULT_GRADUATION_STEP;
> +    }
> +  },
> +
> +  onUpdate(ctx) {

Same comment here, add jsdoc to explains that this is auto-registered, and that it's called in a rAF loop when the highlighter is shown, and that it's used to redraw the rulers.

@@ +54,5 @@
> +    let { width, height, pixelRatio } = this.viewport;
> +    let dpr = getDisplayPixelRatio(this.env.window);
> +
> +    ctx.fillStyle = "rgba(255, 255, 255, 0.8)";
> +    ctx.fillRect(0, 0, width * pixelRatio, 16 * dpr);

Can you move 16 as a const at the top of this file?

@@ +55,5 @@
> +    let dpr = getDisplayPixelRatio(this.env.window);
> +
> +    ctx.fillStyle = "rgba(255, 255, 255, 0.8)";
> +    ctx.fillRect(0, 0, width * pixelRatio, 16 * dpr);
> +    ctx.fillRect(0, 0, 16 * dpr, height * pixelRatio);

Same here

@@ +69,3 @@
>      let { window } = this.env;
> +    let { width, height, pixelRatio } = this.viewport;
> +    let dpr = getDisplayPixelRatio(window);

dpr is already retrieved in onUpdate, can you pass it here instead?
Maybe the cost of getting it is almost free, but remember this is called twice *and* in a rAF loop.

@@ +85,5 @@
> +
> +    let offset = scroll % this.textStep;
> +
> +    for (let i = -offset; i < length; i += this.graduationStep) {
> +      ctx.strokeStyle = "#bebebe";

Please move all colors as const at the top of the file.

@@ +109,3 @@
>  
> +      ctx.fillStyle = "#202020";
> +      ctx.strokeStyle = "#202020";

And these colors.

@@ +112,3 @@
>  
> +      if ((i + offset % this.textStep) === 0 && step > 0) {
> +        ctx.font = `${10 * dpr}px Helvetica`;

And this font too.
At least the "theme" of the rulers will be easier to change this way.

::: devtools/server/actors/highlighters/utils/markup.js
@@ +275,5 @@
>        this.content.removeAttributeForElement(id, name);
>      }
>    },
>  
> +  getCanvasContext(id, type = "2d") {

Please use `getCanvasContext: function(id, type = "2d") {` for better consistency with the rest of the file.
(or create another cleanup commit where you change all methods declarations in this file).

Comment 13

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

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #12)

> Nice, I like the code a lot. Thanks for giving me the opportunity to learn
> about Symbols too.
> One major blocker though: on my windows 10, I see no text at all. The rulers
> are visible, with the graduations, but no labels appear.

The same here with windows 8.1 actually; I thought that at this level, using standard web API I didn't have such issues but I was wrong. I'm going to check what is going on and fix it.

> About tests: I think you could write at least a unit test-like test for the
> CanvasHighlighter class.
> This would be a simple test that instantiates the class, passing it an
> instance of highlighterEnv that you would construct yourself, with a test
> window. You would then test that viewportchange events are emitted when
> needed, and that update events are emitted when needed too, giving you a
> reference to a canvas context you can use.
> The test could also ensure that the update only starts once show is called,
> and stops when hide is called.

Sounds good, not sure however what you mean when you said "updat eevents are emitted when needed too": is there a way to check when requestAnimationFrame are supposed to be called and when they're not? Or I missing something?

> Also, I believe you could also write a unit test-like test for the
> RulersHighlighter class. This one would verify the textStep and
> graduationStep calculations.

That's what I was thinking but I'm not sure it makes sense. What kind of test do you have in mind, specifically?

> > +  setListeners(this, Object.getPrototypeOf(this));
> 
> Wow, I had no idea this existed. This could save us a lot of code right?

Well, it depends. In case of extending class, probably. Specifically, this come from a pattern we had in SDK, where you can already add (custom) events in the object definition. In this case I found that it would be nice instead of having methods like "update", just have "update" events, and give the possibility to add `onUpdate` when we build our object.

> have a feeling we generally don't know enough about the addon SDK. I think
> it would be nice creating a 5 minutes thursday tech talk about the most
> useful helpers. Would you like to do this?

Sure! I have just to find what are the most useful helpers. :)

> Also, why not use this to also register the pagehide event?

pagehide is a DOM event. `setListeners` works only for custom event, emitted directly to the object itself.

> @@ +95,5 @@
> > +        width: innerWidth,
> > +        height: innerHeight,
> > +        pixelRatio: devicePixelRatio
> > +      });
> > +      this[_viewport] = viewport;
> 
> I found it a little bit confusing that you're accessing the viewport via
> both the getter and the symbol. I was originally going to say that you
> should remove the getter and just use the symbol everywhere, but since you
> need to getter in rulers.js, then please just use the getter everywhere
> instead and add a setter.

But that's the whole point: `viewport` should be read-only, that's why there is only the getter. From the subclass you're able to access to it, but not to modify it.
However, I can definitely use only the Symbol inside the `canvas.js` module to make it more clear.
 
> @@ +69,3 @@
> >      let { window } = this.env;
> > +    let { width, height, pixelRatio } = this.viewport;
> > +    let dpr = getDisplayPixelRatio(window);
> 
> dpr is already retrieved in onUpdate, can you pass it here instead?
> Maybe the cost of getting it is almost free, but remember this is called
> twice *and* in a rAF loop.

I can pass as argument of `drawAxis`, yes, I thought it was cleaner in that way but I don't have strong feeling about that.

Comment 14

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

(In reply to Matteo Ferretti [:matteo][:zer0] from comment #13)

> > One major blocker though: on my windows 10, I see no text at all. The rulers
> > are visible, with the graduations, but no labels appear.
> 
> The same here with windows 8.1 actually; I thought that at this level, using
> standard web API I didn't have such issues but I was wrong. I'm going to
> check what is going on and fix it.

OK, apparently I have messed with the branches, 'cause if I get a clean branch on OS X and I apply this patch I do not have text as well; where if I use the local branch from where I generate the patch I have them. That's shame for me, but at least a good news 'cause it means is not some nasty platform-specific bug.

Comment 15

[Patrick Brosset <:pbro>]

Filter on CLIMBING SHOES