Closed Bug 1150814 Opened 9 years ago Closed 9 years ago

Rulers graduations are not displayed

Categories

(DevTools :: Inspector, defect)

x86
Windows 7
defect
Not set
normal

Tracking

(firefox40 affected, firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox40 --- affected
firefox42 --- fixed

People

(Reporter: zer0, Assigned: zer0)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

It seems that the CSS workaround used to have 1px stroke width for the graduations, is broken in some platform. As result, the graduations are not displayed at all.
Assignee: nobody → zer0
Attached patch fix-rulers-stroke.patch (obsolete) — Splinter Review
Attachment #8587855 - Flags: review?(pbrosset)
Comment on attachment 8587855 [details] [diff] [review]
fix-rulers-stroke.patch

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

::: toolkit/devtools/server/actors/highlighter.js
@@ +2634,5 @@
> +
> +  this._zoom = 0;
> +
> +  this._cancelUpdate();
> +  this._update();

Why call both _cancelUpdate and _update here?
_cancelUpdate will have no effect and _update will do a bunch of stuff, including changing markup and starting the rAF loop, even though the highlighter is not shown yet.
You should only do all of this in show, and cancel the update in hide.

@@ +2810,5 @@
> +
> +  update: function() {
> +    let prefix = this.ID_CLASS_PREFIX;
> +    let { documentElement } = this.win.document;
> +    let zoom = LayoutHelpers.getCurrentZoom(documentElement);

I think LayoutHelpers.getCurrentZoom(this.win) should work just as well, no need to get the documentElement if I remember correctly.
Attachment #8587855 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #2)

> @@ +2810,5 @@
> > +
> > +  update: function() {
> > +    let prefix = this.ID_CLASS_PREFIX;
> > +    let { documentElement } = this.win.document;
> > +    let zoom = LayoutHelpers.getCurrentZoom(documentElement);
> 
> I think LayoutHelpers.getCurrentZoom(this.win) should work just as well, no
> need to get the documentElement if I remember correctly.

You remember correctly; however I used `documentElement` here 'cause I need it anyway later for the `setIgnoreLayoutChanges` call. If you prefer, I can just pass `this.win` here.
Attachment #8587855 - Attachment is obsolete: true
Comment on attachment 8639315 [details] [diff] [review]
Rulers graduations are not displayed

I improved a bit the previous patch; plus I added a check in gcli in case the window is closed / destroyed, to avoid exceptions.
Attachment #8639315 - Flags: review?(pbrosset)
Blocks: 1152330
Comment on attachment 8639315 [details] [diff] [review]
Rulers graduations are not displayed

Changing reviewer 'cause Patrick will be on PTO for a while.

Mike, can you have a look at that? If you don't feel comfortable to review this, just let me know! Thanks!
Attachment #8639315 - Flags: review?(pbrosset) → review?(mratcliffe)
Attachment #8639315 - Flags: review?(mratcliffe) → review+
Comment on attachment 8644799 [details] [diff] [review]
Rulers graduations are not displayed

Just edited the commit's comment appending "r=mratcliffe" instead of patrick
Attachment #8644799 - Flags: review+
Attachment #8639315 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1102281dff11
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Reproduce this bug on Firefox nightly Version 40.0a1

The bug is verified and fix on Latest Nightly

Build ID 	20150826030211
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0

Tested OS -- Windows 8.1 32bit
QA Whiteboard: [bugday-20150826]
Should the Rulers be changed direction in the RTL locale version? If yes, it should be changed.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: