Closed
Bug 1042619
Opened 8 years ago
Closed 7 years ago
In Inspector, box model view, use the sign × instead of the letter x
Categories
(DevTools :: Inspector, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 38
People
(Reporter: nicolas.barbulesco, Assigned: aaronraimist)
References
Details
Attachments
(1 file, 4 obsolete files)
21.13 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8540478 -
Flags: review?(dcamp)
Updated•8 years ago
|
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 3•8 years ago
|
||
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+
Comment 4•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/291e3a83a122
Whiteboard: [fixed-in-fx-team]
Comment 5•8 years ago
|
||
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]
Comment 6•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
(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
Assignee | ||
Comment 8•8 years ago
|
||
Flags: needinfo?(aaronraimist)
Assignee | ||
Updated•8 years ago
|
Attachment #8550913 -
Flags: review?(bgrinstead)
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
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
Assignee | ||
Comment 11•7 years ago
|
||
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
Comment 12•7 years ago
|
||
(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.
Comment 13•7 years ago
|
||
(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.
Updated•7 years ago
|
Attachment #8540478 -
Attachment is obsolete: true
Assignee | ||
Comment 14•7 years ago
|
||
I used × in the tests and the escape code in everything else.
Attachment #8552734 -
Flags: review?(bgrinstead)
Updated•7 years ago
|
Attachment #8550913 -
Attachment is obsolete: true
Comment 15•7 years ago
|
||
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff11ceed1ff2
Assignee | ||
Comment 16•7 years ago
|
||
(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
Comment 17•7 years ago
|
||
(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?
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8552734 -
Attachment is obsolete: true
Attachment #8552734 -
Flags: review?(bgrinstead)
Attachment #8553466 -
Flags: review?(bgrinstead)
Comment 19•7 years ago
|
||
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+
Comment 20•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8554829 -
Flags: review+
Updated•7 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Attachment #8553466 -
Attachment is obsolete: true
Comment 22•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0e5635270d32
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0e5635270d32
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•