Last Comment Bug 657143 - Reorder mature CSS properties in nsComputedDOMStyle.cpp
: Reorder mature CSS properties in nsComputedDOMStyle.cpp
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Ian Moody [:Kwan]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 1043461
  Show dependency treegraph
 
Reported: 2011-05-14 09:33 PDT by Ian Moody [:Kwan]
Modified: 2014-07-24 09:35 PDT (History)
5 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reorder properties in nsComputedDOMStyle.cpp as described (14.30 KB, patch)
2011-05-14 13:41 PDT, Ian Moody [:Kwan]
bzbarsky: review+
Details | Diff | Splinter Review
add test and fix some missed props and new -moz-orient (8.40 KB, patch)
2011-05-18 16:09 PDT, Ian Moody [:Kwan]
bzbarsky: review+
Details | Diff | Splinter Review
add test and fix some missed props and new -moz-orient v1.1 (7.78 KB, patch)
2011-05-19 14:45 PDT, Ian Moody [:Kwan]
no flags Details | Diff | Splinter Review

Description Ian Moody [:Kwan] 2011-05-14 09:33:53 PDT
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
Comment 1 Ian Moody [:Kwan] 2011-05-14 13:41:39 PDT
Created attachment 532465 [details] [diff] [review]
Reorder properties in nsComputedDOMStyle.cpp as described
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-05-15 11:07:14 PDT
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?
Comment 3 Ian Moody [:Kwan] 2011-05-15 14:06:30 PDT
Yes that would be great thank you.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-16 16:22:34 PDT
http://hg.mozilla.org/projects/cedar/rev/cf2a754bf3cb
Comment 5 Mounir Lamouri (:mounir) 2011-05-17 06:38:26 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/cf2a754bf3cb
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-05-17 10:40:05 PDT
fwiw, we could add a test for this, since this ordering is detectable from script.
Comment 7 Ian Moody [:Kwan] 2011-05-18 16:07:47 PDT
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.
Comment 8 Ian Moody [:Kwan] 2011-05-18 16:09:53 PDT
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.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-05-19 13:03:41 PDT
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.
Comment 10 Ian Moody [:Kwan] 2011-05-19 14:45:51 PDT
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.
Comment 11 Mounir Lamouri (:mounir) 2011-05-20 06:59:28 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/abbf75ba7b59
Comment 12 Ian Moody [:Kwan] 2011-06-01 08:33:08 PDT
Verified this on Aurora earlier.  Thank you very much Boris, Robert and Mounir for all your help!

Note You need to log in before you can comment on or make changes to this bug.