Closed Bug 1042619 Opened 5 years ago Closed 5 years ago

In Inspector, box model view, use the sign × instead of the letter x

Categories

(DevTools :: Inspector, defect, minor)

All
Windows 7
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: nicolas.barbulesco, Assigned: aaronraimist)

References

Details

Attachments

(1 file, 4 obsolete files)

Firefox 31, on Win 7.

The box model view of the Inspector is (well) described here : 
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector#Box_model_view

In the Inspector, we have sizes of boxes like this : 
"240x125" with the letter x of the latin alphabet

I think it would be better to have the sizes like this : 
"240×125" with the multiplication sign ×

It would be more correct. The intented meaning here is the sign "times". The letter x has nothing to do here, it just landed here by a typographic accident of history.

The similar bug 677912 in Firefox has been fixed. 
Correcting this issue in the Inspector of Firefox would be consistent. 

Google Chrome's Inspector gives sizes of boxes like this : 
"274px × 105px"

Thank you.
Attached patch bug1042619_timessign.diff (obsolete) — Splinter Review
Attachment #8540478 - Flags: review?(dcamp)
Assignee: nobody → aaronraimist
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8540478 [details] [diff] [review]
bug1042619_timessign.diff

Redirecting to bgrins.
Attachment #8540478 - Flags: review?(dcamp) → review?(bgrinstead)
Comment on attachment 8540478 [details] [diff] [review]
bug1042619_timessign.diff

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

Looks good.  I'd like to add a shared.properties file and so we can localize strings like this (and other commonly used strings across tools), but I'll file a follow up bug for that.
Attachment #8540478 - Flags: review?(bgrinstead) → review+
So apparently using the 'x' character in a commit message breaks builds (Bug 1121132).  Also, I suspect this may cause some test failures - I'll push this to try.

Backed out: https://hg.mozilla.org/integration/fx-team/rev/268dfa4925ec
Whiteboard: [fixed-in-fx-team]
There are some tests that are failing with this applied.  For instance in the layoutview there are things checking for 100x150, etc:

https://dxr.mozilla.org/mozilla-central/source/browser/devtools/layoutview/test/browser_layoutview.js#12
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/layoutview/test/browser_layoutview_update-after-navigation.js#48

Can you update the tests to look for the × character instead?  You can run the tests locally to check with ./mach mochitest-devtools browser/devtools/layoutview/

Here is a try push, which will have full test results when it finishes: https://tbpl.mozilla.org/?tree=Try&rev=031dc11ff815
Flags: needinfo?(aaronraimist)
(In reply to Brian Grinstead [:bgrins] from comment #6)
> Here is a try push, which will have full test results when it finishes:
> https://tbpl.mozilla.org/?tree=Try&rev=031dc11ff815

There are also changes needed for the canvas debugger and responsive UI tests that can be seen in the link
Flags: needinfo?(aaronraimist)
Attachment #8550913 - Flags: review?(bgrinstead)
Comment on attachment 8550913 [details] [diff] [review]
bug1042619_width×height_devtools_v2.diff

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

Can we just use the symbol - × - throughout these files?  It will be a lot easier to read.
Attachment #8550913 - Flags: review?(bgrinstead)
Also, I'm still seeing a failure in browser/devtools/netmonitor/test/browser_net_content-type.js and browser/devtools/markupview/test/browser_markupview_image_tooltip.js.  Relevant try push: https://tbpl.mozilla.org/?tree=Try&rev=72372b29ec5c
I can do that for everything except the responsive design view. In the responsive design view dropdown menu I get unknown character boxes. Do you know what I would need to change to make it work without using escape sequences?

http://i.imgur.com/k8ltCg8.png
http://i.imgur.com/YdNAwT1.png
(In reply to Aaron Raimist [:aaronraimist] from comment #11)
> I can do that for everything except the responsive design view. In the
> responsive design view dropdown menu I get unknown character boxes. Do you
> know what I would need to change to make it work without using escape
> sequences?

That's because the file is saved as charset=us-ascii while the other files are saved with charset=utf-8.  So, you'll need to save that file with UTF8 encoding.
(In reply to Brian Grinstead [:bgrins] from comment #12)
> (In reply to Aaron Raimist [:aaronraimist] from comment #11)
> > I can do that for everything except the responsive design view. In the
> > responsive design view dropdown menu I get unknown character boxes. Do you
> > know what I would need to change to make it work without using escape
> > sequences?
> 
> That's because the file is saved as charset=us-ascii while the other files
> are saved with charset=utf-8.  So, you'll need to save that file with UTF8
> encoding.

I was wrong about that - it still shows up this way even when resaving.  It may have to do with the charset on the browser xul file.  Either way, I'd say let's switch back to the escape code, but only for the 3 files in the original patch (canvasdebugger.js | view.js | responsivedesign.jsm).  Keep the tests using the symbol.
Attachment #8540478 - Attachment is obsolete: true
I used × in the tests and the escape code in everything else.
Attachment #8552734 - Flags: review?(bgrinstead)
Attachment #8550913 - Attachment is obsolete: true
(In reply to Brian Grinstead (unavailable through 1-26) [:bgrins] from comment #15)
> Pushed to try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff11ceed1ff2

Looks like the symbol doesn't work in the tests either:

 INFO TEST-UNEXPECTED-FAIL | browser/devtools/markupview/test/browser_markupview_image_tooltip.js | Tooltip label for [img.local] displays the right image size - Got 192 × 192, expected 192 × 192
(In reply to Aaron Raimist [:aaronraimist] from comment #16)
> (In reply to Brian Grinstead (unavailable through 1-26) [:bgrins] from
> comment #15)
> > Pushed to try:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff11ceed1ff2
> 
> Looks like the symbol doesn't work in the tests either:
> 
>  INFO TEST-UNEXPECTED-FAIL |
> browser/devtools/markupview/test/browser_markupview_image_tooltip.js |
> Tooltip label for [img.local] displays the right image size - Got 192 × 192,
> expected 192 × 192

Darn, sorry about that.  Can you replace those instances with the escape code instead?
Attachment #8552734 - Attachment is obsolete: true
Attachment #8552734 - Flags: review?(bgrinstead)
Attachment #8553466 - Flags: review?(bgrinstead)
Comment on attachment 8553466 [details] [diff] [review]
bug1042619_width×height_devtools_v4.diff

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

Try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9e331dec924
Attachment #8553466 - Flags: review?(bgrinstead) → review+
The commit message will need to be changed before landing this to remove the unicode symbol due to Bug 1121132.  Can you replace the × in the commit message with 'a multiplication sign'?
Flags: needinfo?(aaronraimist)
This should be good.
Flags: needinfo?(aaronraimist)
Attachment #8554829 - Flags: review+
Attachment #8553466 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0e5635270d32
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.