Last Comment Bug 665857 - computed 'list-style-type' value is the empty string for ordered lists
: computed 'list-style-type' value is the empty string for ordered lists
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P4 normal (vote)
: mozilla7
Assigned To: David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-21 03:48 PDT by Daniel Glazman (:glazou)
Modified: 2011-08-30 08:51 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test for type attribute on ol and li (1.52 KB, text/html)
2011-06-21 10:05 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
patch (11.79 KB, patch)
2011-06-21 11:29 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
bzbarsky: review+
Details | Diff | Splinter Review

Description Daniel Glazman (:glazou) 2011-06-21 03:48:26 PDT
given the following markup

<ol><li id="foo" type="i">aaa</li><li>bbbbb</li></ol>

the following code

document.defaultView.
  getComputedStyle(document.getElementById("foo"), null).
  getPropertyValue("list-style-type")

replies "" while it should really reply "lower-roman", right ? Happens with
other ordered values of the type attribute. Unordered seem ok AFAICT.
Comment 1 Boris Zbarsky [:bz] 2011-06-21 09:22:42 PDT
<li type="i"> doesn't use lower-roman at the moment.  It uses an internal computed value of list-style-type that's not expressible as a CSS specified value.

Same for the other ordered values of @type on <li>.

David, any idea why we have those?  nsBulletFrame seems to treat them the same as the CSS values....
Comment 3 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-06-21 09:55:34 PDT
I'll remove them.
Comment 4 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-06-21 10:05:10 PDT
Created attachment 540791 [details]
test for type attribute on ol and li

There are a bunch of values of type that we support but other browsers don't.
Comment 5 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-06-21 10:10:23 PDT
Boy, that's a wonderful picture of interop on 15-20-year-old technology, there.

I was thinking of ripping out support for some of those un-specced type attribute values, but I think I'll leave that can of worms for another day.
Comment 6 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-06-21 11:29:49 PDT
Created attachment 540813 [details] [diff] [review]
patch

As expected, 9 of the 10 tests fail without the patch.
Comment 7 Boris Zbarsky [:bz] 2011-06-21 13:19:00 PDT
Comment on attachment 540813 [details] [diff] [review]
patch

Hmm.  For the OListElement case, do we have any tests to make sure that parsing and then serializing <ol type="i"> gives <ol type="i"> instead of <ol type="lower-roman"> and vice versa?  That used to be an issue, but we might store the string with enumerated values now...

r=me assuming that part works correctly.
Comment 8 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-06-21 13:53:15 PDT
(In reply to comment #5)
> Boy, that's a wonderful picture of interop on 15-20-year-old technology,
> there.
> 
> I was thinking of ripping out support for some of those un-specced type
> attribute values, but I think I'll leave that can of worms for another day.

I filed bug 666017 on this.
Comment 9 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-06-21 17:22:25 PDT
(In reply to comment #7)
> Comment on attachment 540813 [details] [diff] [review] [review]
> patch
> 
> Hmm.  For the OListElement case, do we have any tests to make sure that
> parsing and then serializing <ol type="i"> gives <ol type="i"> instead of
> <ol type="lower-roman"> and vice versa?  That used to be an issue, but we
> might store the string with enumerated values now...
> 
> r=me assuming that part works correctly.

We no longer even have any code for converting enumerated types back to value strings, so I'm pretty sure we store the string.  I spot-checked that both "i" and "lower-roman" are preserved (parse -> getAttribute) on ul (and that we even preserve case of the latter), but I don't feel the need to add automated tests for this.
Comment 10 Boris Zbarsky [:bz] 2011-06-21 19:55:04 PDT
OK, sounds good.  Thank you for checking!
Comment 11 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-06-29 16:52:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e74c4c9f1c1c
Comment 12 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-06-30 06:02:49 PDT
http://hg.mozilla.org/mozilla-central/rev/e74c4c9f1c1c
Comment 13 Virgil Dicu [:virgil] [QA] 2011-08-26 01:21:06 PDT
Could anyone provide a test case for this issue in order to verify it?
Comment 14 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-08-26 07:41:19 PDT
There's one contained in the patch that's for each platform for each push to hg (except when multiple pushes get coalesced into a single test run).  There's also one in comment 0.

But why do you need it, anyway?  What's the purpose of the verification that you're trying to do, and is that purpose met by simply running a testcase provided by somebody else?  (In my opinion, that type of bug verification is only useful for bus that absolutely must be fixed in a particular release, like security bugs.  I think, otherwise, the main use of bug verification is to test that all related cases have been fixed, and no other related bugs have been caused, which requires significant understanding of the technology being fixed (in this case, HTML and CSS).)
Comment 15 Virgil Dicu [:virgil] [QA] 2011-08-30 08:51:51 PDT
Verified fixed based on the results in
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1314705647.1314705993.17798.gz

(In reply to David Baron [:dbaron] from comment #14)
> There's one contained in the patch that's for each platform for each push to
> hg (except when multiple pushes get coalesced into a single test run). 
> There's also one in comment 0.
> 
> But why do you need it, anyway?  What's the purpose of the verification that
> you're trying to do, and is that purpose met by simply running a testcase
> provided by somebody else?  (In my opinion, that type of bug verification is
> only useful for bus that absolutely must be fixed in a particular release,
> like security bugs.  I think, otherwise, the main use of bug verification is
> to test that all related cases have been fixed, and no other related bugs
> have been caused, which requires significant understanding of the technology
> being fixed (in this case, HTML and CSS).)

I was checking this issue as part of my tasks in QA-currently concentrating on verifying F7 milestone issues . As for this task, I found a test case preferable to other checks. 

I understand your position regarding this issue.

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