The default bug view has changed. See this FAQ.

Reorder mature CSS properties in nsComputedDOMStyle.cpp

VERIFIED FIXED in mozilla6

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Kwan, Assigned: Kwan)

Tracking

Trunk
mozilla6
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 

Originally the CSS properties in nsComputedDOMStyle.cpp were listed in alphabetical order, and then experimental properties prefixed with -moz- were kept separate at the bottom.  Several of these properties have now matured/stabilised enough to drop the prefix and become fully supported, yet they are still in the list of experimental properties.  While this may not seem a particularly big deal there is one area where is has a noticeable effect: DOM Inspector's computed style panel, which seems to be ordered by this file.  As such when inspecting an elements style it often becomes annoying trying to locate properties that are not where you would expect to find them, and then having to scroll up and down to check properties that should be next to each other (ie background-*).

Reproducible: Always

Steps to Reproduce:
1. Install the DOM Inspector
2. Inspect an element in a web page
3. Check out its computed style
4. Properties are not where expected/have to scroll up and down to check different background-* properties

Actual Results:  
mature properties should be in alphabetical order, and then -moz- properties below them in their own order

Expected Results:  
Some matures properties are mixed in among the -moz- ones, and some of the -moz- ones are not in order within -moz-

Other improvements:
*Bump the SVG specific styles down into their own block with a comment at the top so future non-SVG styles are not mixed in with them (as currently happened with -moz-text-blink and -moz-text-decoration-*), plus they are no longer experimental anyway
*Change CSS2 to just CSS in the comment block at the top of the list, since it is no longer purely CSS2 properties but CSS3 as well

Have preliminary patch for m-c, will attach shortly
I assume its too late to take a similar patch for aurora/fx5, but if not I'd be happy to put one together
(Assignee)

Comment 1

6 years ago
Created attachment 532465 [details] [diff] [review]
Reorder properties in nsComputedDOMStyle.cpp as described
Attachment #532465 - Flags: review?(bzbarsky)
Assignee: nobody → moz-ian
Severity: trivial → normal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Comment on attachment 532465 [details] [diff] [review]
Reorder properties in nsComputedDOMStyle.cpp as described

r=me

> I assume its too late to take a similar patch for aurora/fx5

Yep.

Please let me know if you want me to push the patch for you, ok?
Attachment #532465 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 3

6 years ago
Yes that would be great thank you.
Keywords: checkin-needed
http://hg.mozilla.org/projects/cedar/rev/cf2a754bf3cb
Keywords: checkin-needed
Whiteboard: fixed-in-cedar
Pushed:
http://hg.mozilla.org/mozilla-central/rev/cf2a754bf3cb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla6
fwiw, we could add a test for this, since this ordering is detectable from script.
Flags: in-testsuite- → in-testsuite?
(Assignee)

Comment 7

6 years ago
Reopened as I missed some singletons and the new -moz-orient was put into the wrong list pretty much straight after :D. Patch with test incoming.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

6 years ago
Created attachment 533456 [details] [diff] [review]
add test and fix some missed props and new -moz-orient

Okay figured out your test framework and made this, which also showed some I'd missed as they weren't obviously out of place (except ime-mode, that's just my fail) so fixed those as well.  roc kindly tested it for me on a local build of his (and fixed a couple of my mistakes) where it errored on the new -moz-orient in the SVG list, but the error message wasn't that helpful so I reworked it to be better in that case, and fixed -moz-orient.  I've tested it as best I can locally but can't build so can't verify that it definitely gives the right error for -moz- in SVG, but I've looked at the code pretty hard and don't see why it wouldn't.
Attachment #533456 - Flags: review?(bzbarsky)
Comment on attachment 533456 [details] [diff] [review]
add test and fix some missed props and new -moz-orient

>+    if (prop == 'clip-path') {

Need to push that into svgA.

Other than that, looks good.
Attachment #533456 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

6 years ago
Created attachment 533800 [details] [diff] [review]
add test and fix some missed props and new -moz-orient v1.1

Fixed comments.
Ah, of course it does.  Thank you.
Attachment #533456 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/abbf75ba7b59
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
(Assignee)

Comment 12

6 years ago
Verified this on Aurora earlier.  Thank you very much Boris, Robert and Mounir for all your help!
Status: RESOLVED → VERIFIED
Blocks: 1043461
You need to log in before you can comment on or make changes to this bug.