Closed Bug 1084442 Opened 10 years ago Closed 10 years ago

New highlighter implementation (canvasframe anon content) is incorrectly positioned when the page is zoomed

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(1 file, 2 obsolete files)

The new highlighter landing with bug 985597 uses the canvasFrame's native anonymous node to insert its DOM.

This means that zooming in/out the page will zoom in/out the highlighter too!

This is ... interesting ... And also, I totally did not see this coming in bug 1020244 (which is the platform API).

I don't think there's a good solution for this. It's probably not a very important problem anyway.

What's more important is that the highlighter doesn't seem to be properly positioned when the page is zoomed.
This we should fix (probably in LayoutHelpers.jsm)
Depends on: 985597
+1 for maybe fixing the positioning. Otherwise I'm not very interested in this bug unless we get feedback or ( off the top of my head ) if this somehow makes accessibility for the tools worse.
This might be worth asking someone who knows more about layout than me, but one way I see this working is the following:

let winUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
                     .getInterface(Ci.nsIDOMWindowUtils);
let zoom = winUtils.fullZoom;
let scale = 1/zoom;
highlighter.style.transform = "scale("+scale+")";
highlighter.style.transformOrigin = "top left";

The idea here is that, whenever the page is zoomed in or out, we apply the inverse scale transform on the highlighter so that it remains un-zoomed.
Working on a patch.
Assignee: nobody → pbrosset
Before I go ahead and polish the code and add tests, I'd like some feedback on the approach used here.

Mike, you've worked a lot on the highlighter too, what do you think of this?

For info, the important change here is the use of "transform:scale(1/zoom)" to make sure the highlighter isn't zoomed in or out with the page.

Brian: I'm interested in your feedback too, there could be other ways to achieve this.
Attachment #8524582 - Flags: feedback?(mratcliffe)
Attachment #8524582 - Flags: feedback?(bgrinstead)
Another use case came up while talking to smaug on IRC: the way firefox used to zoom pages for a long time was text-only zoom. And it turns out this way still exists. If you go to the "View" menu and then "Zoom", you can check the "Zoom only text" item, which reverts firefox to text-only zoom.

My previous patch uses the fullZoom attribute from nsIDOMWindowUtils and text-only zoom doesn't change this attribute. But still the highlighter is correctly positioned, because only the text size was changed, most of the highlighter's elements are absolutely positioned, using px, so they aren't impacted by the zoom.
However the text inside the node-infobar is updated by this textZoom change.

I looks like the textZoom value can be retrieved via docShell.contentViewer.textZoom.
I suggest that this gets fixed in a second patch or second bug. There are probably far less people using text-only zoom since fullZoom is the default, and even though, the highlighter is correctly positioned, just the text is bigger/smaller.
Comment on attachment 8524582 [details] [diff] [review]
bug1084442-zoomed-highlighter v1.patch

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> Another use case came up while talking to smaug on IRC: the way firefox used
> to zoom pages for a long time was text-only zoom. And it turns out this way
> still exists. If you go to the "View" menu and then "Zoom", you can check
> the "Zoom only text" item, which reverts firefox to text-only zoom.
> 
> My previous patch uses the fullZoom attribute from nsIDOMWindowUtils and
> text-only zoom doesn't change this attribute. But still the highlighter is
> correctly positioned, because only the text size was changed, most of the
> highlighter's elements are absolutely positioned, using px, so they aren't
> impacted by the zoom.
> However the text inside the node-infobar is updated by this textZoom change.
> 
> I looks like the textZoom value can be retrieved via
> docShell.contentViewer.textZoom.
> I suggest that this gets fixed in a second patch or second bug. There are
> probably far less people using text-only zoom since fullZoom is the default,
> and even though, the highlighter is correctly positioned, just the text is
> bigger/smaller.

My first thought was about text-only zoom. Not sure what can be done about the nodeInfoBar inheriting textZoom. Is that really a problem? I mean, if somebody has zoomed the text they probably prefer a larger / smaller text size anyway. If we really need to fix it we could just render the text in a canvas instead and that should preserve it's size (if that is possible inside the canvasFrame).

The approach you take in your patch should work well with zoomed windows. Because canvasFrame lives inside the current page I don't think there is another approach... not a simple one anyway.

We used to have a test for the highlighter on zoomed windows but I think it was removed because we assumed it would just work, that test should be added again.
Attachment #8524582 - Flags: feedback?(mratcliffe) → feedback+
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #7)
> My first thought was about text-only zoom. Not sure what can be done about
> the nodeInfoBar inheriting textZoom. Is that really a problem? I mean, if
> somebody has zoomed the text they probably prefer a larger / smaller text
> size anyway.
That is a really good point Mike. So let's leave the textZoom part for a follow-up bug if it does become a problem. But I suspect it won't.
(In reply to Jeff Griffiths (:canuckistani) from comment #1)
> +1 for maybe fixing the positioning. Otherwise I'm not very interested in
> this bug unless we get feedback or ( off the top of my head ) if this
> somehow makes accessibility for the tools worse.

I think this is actually quite an important fix that should land in the same release as the remote highlighter.  The highlighter isn't working for anyone who zooms in (or out) on the page.
Comment on attachment 8524582 [details] [diff] [review]
bug1084442-zoomed-highlighter v1.patch

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

Overall the changes seem sound - I'm not sure I fully understand some of the changes to width/height, and I think that some of this is duplicated between CSS and JS, but it all looks reasonable and fixes the zoom problem.  Will this work fine with nested iframes?

::: toolkit/devtools/server/actors/highlighter.css
@@ +25,5 @@
>  }
>  
>  /* Box model highlighter */
>  
> +:-moz-native-anonymous .box-model-root {

isn't the width: 100%; height: 100% going to be set in unzoomElement?  If the position absolute is actually necessary here, I'm thinking it'd be easier to just go ahead and also set position:absolute there and get rid of this duplication in the CSS.  That way unzoomElement would work on any root without needing to have some special CSS applied to the root.

::: toolkit/devtools/server/actors/highlighter.js
@@ +495,5 @@
> +   * with this API to remain unzoomed, then this method can be used.
> +   *
> +   * Note that the element that the matching element will be scaled down or
> +   * up by 1/zoom (using css transform) to cancel the current zoom.
> +   * The element's width and height styles can also optionally be set.

Is there a case where we *don't* want to set the width and height?  I'm not seeing any usage in this patch, so unless if you can think of a use case that we'd immediately want, I would suggest removing this as an option to get rid of extra complexity here.

@@ +513,5 @@
> +
> +    if (zoom !== 1) {
> +      value = "transform-origin:top left;transform:scale(" + (1/zoom) + ");";
> +      if (resize) {
> +        value += "width:" + (100*zoom) + "%;height:" + (100*zoom) + "%;";

This seems to work fine, I'm not positive why we need both transform and width / height, but it works.  Can you explain why?
Attachment #8524582 - Flags: feedback?(bgrinstead) → feedback+
(In reply to Brian Grinstead [:bgrins] from comment #10)
> ::: toolkit/devtools/server/actors/highlighter.css
> @@ +25,5 @@
> >  }
> >  
> >  /* Box model highlighter */
> >  
> > +:-moz-native-anonymous .box-model-root {
> 
> isn't the width: 100%; height: 100% going to be set in unzoomElement?  If
> the position absolute is actually necessary here, I'm thinking it'd be
> easier to just go ahead and also set position:absolute there and get rid of
> this duplication in the CSS.  That way unzoomElement would work on any root
> without needing to have some special CSS applied to the root.
Yeah, I think you're right, I just changed the patch to have position:absolute set by the unzoomElement method and removed the selector altogether from the CSS.

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +495,5 @@
> > +   * with this API to remain unzoomed, then this method can be used.
> > +   *
> > +   * Note that the element that the matching element will be scaled down or
> > +   * up by 1/zoom (using css transform) to cancel the current zoom.
> > +   * The element's width and height styles can also optionally be set.
> 
> Is there a case where we *don't* want to set the width and height?  I'm not
> seeing any usage in this patch, so unless if you can think of a use case
> that we'd immediately want, I would suggest removing this as an option to
> get rid of extra complexity here.
No there isn't such a case, so I've removed this option.
I guess my original idea for unzoomElement was to make it more generic, but it turns out it's only useful right now to unzoom the root wrapper element in a highlighter. So there's no sense making it do anything else and have options for this.
I might as well rename it something like scaleWrapperElement and adapt the comment to make it very obvious what it's for.

> @@ +513,5 @@
> > +
> > +    if (zoom !== 1) {
> > +      value = "transform-origin:top left;transform:scale(" + (1/zoom) + ");";
> > +      if (resize) {
> > +        value += "width:" + (100*zoom) + "%;height:" + (100*zoom) + "%;";
> 
> This seems to work fine, I'm not positive why we need both transform and
> width / height, but it works.  Can you explain why?
This is required because scaling a 100%/100% element with scale(.5) for instance makes it take 50%/50% of the viewport. So on top of scaling to make sure the highlighter keeps the same aspect ratio independent of which zoom level is chosen, I also need to change the width/height so it covers the whole viewport.
Thanks Mike and Brian for the feedback.
I believe this new patch takes your comments into account. In particular, here are the main changes from v1:
- added a new, zoom-specific, test to our test suite
- removed the position:absolute;width:100%;height:100%; from the CSS on both root elements, since this is handled from JS
- simplified a little bit and renamed unzoomElement, it's now named scaleRootElement
- also improved the comment for this method
Attachment #8524582 - Attachment is obsolete: true
Attachment #8526116 - Flags: review?(mratcliffe)
I forgot to update a few highlighter element IDs in tests after my latest changes. That's why the last try push shows failures.
The tests pass fine locally now.
I will push to try again as soon as the try opens again.
Attachment #8526116 - Attachment is obsolete: true
Attachment #8526116 - Flags: review?(mratcliffe)
Attachment #8526278 - Flags: review?(mratcliffe)
Attachment #8526278 - Flags: review?(mratcliffe) → review+
Thanks Mike for the quick review. Here's a new try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c0f23ed87901
Status: NEW → ASSIGNED
For info, I've noticed that when zooming the page, the nodeinfobar arrow sometimes gets split in 2, in the middle, by a vertical line.
I've filed bug 1102824 for this.
https://hg.mozilla.org/mozilla-central/rev/67a565ca21d6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.