145311 - [PATCH] Problem to set CSS style of OL to "list-style-type: upper-*;"

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[Christian Tanguy]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.0rc2) Gecko/20020510
BuildID:    2002051006

You cannot set CSS style to "list-style-type: upper-alpha;" (or upper-roman) for
a OL list, except by "HTML source"-editing

Reproducible: Always
Steps to Reproduce:
1. Check "Use CSS instead of ..."
2. Open a new composer page
3. Create a list (from toolbar) and create a few items
4. Do Format/List/List properties
5. Choose Number Style="A, B, C", and  "change entire list"


Actual Results:  You get the "a, b, c" style instead !


Expected Results:  "A, B, C"

Comment 1

[Jonathan Gardner]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.1a) Gecko/20020610

Verified.

One note: To do step (1) above, you must go to Edit->Prefences->Composer.

I made two lists, the first without CSS, and the second with. This is the code
it produced:

<ol type="A">
  <li>asdf</li>
  <li>asdf</li>
</ol>
<br>
<ol style="list-style-type: lower-alpha;">
  <li>asdf</li>
  <li>asdf</li>
</ol>

Obviously, lower-alpha should be upper-alpha.

Comment 2

[Jonathan Gardner]

I am trying to look at the code, but I am coming up empty-handed.

I noticed that when you select "A, B, C" it sets the type to "A", and "a, b, c"
sets it to "a". When it translates that into css style types, it looks for "a"
first, and if it matches, it uses "lower-alpha". Then it tries to match "A", and
if that matches, it sets it to "upper-alpha".

My thought is that the matching routine is not working as expected. The coder
expects that the difference between "A" and "a" will be preserved. This is not
the case, because in this routine, everything is put into lower case.

http://lxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLCSSUtils.cpp#818


Notice that you cannot set it to upper-roman ("I") either. That sets it to
lower-roman ("i").

Comment 3

[Jonathan Gardner]

Attachment: Context diff -- fixes case insensitivity

This patch removes the line that puts all the values into lower case as it is
being converted into CSS.

Note that in HTML, <ol type="A"> is different from <ol type="a">. In CSS, the
first should be interpreted as <ol style="list-style-type: upper-roman">.

Now, I hope this patch DOESN'T get included AS-IS. There are going to be some
side effects that I don't know about. Is there a good reason why the values
were being put into lower-case in the first place? My thought is that the
programmer wanted to do a quick case-insensitive Equals later on in the
Process* functions. Isn't there a case insensitive Equals function somewhere
that can be used?

Comment 4

[Kathleen :Brade]

-->glazman for review

Comment 5

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Jonathan found the root of the problem but his way of solving it is a bit too...
let's say rude :-)
Christian : merci pour avoir détecté ça.
Jonathan  : thanks for tracking
Kathy     : thanks for Cc:ing

Accepting bug.

Comment 6

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 88026 [details] [diff] [review]
Context diff -- fixes case insensitivity

this is a too strong solution; taking over

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.0

Comment 8

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Kathy, Kin, can you r/sr please ?

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: context diff patch, sorry for forgetting about the -u

Comment 10

[Kathleen :Brade]

Comment on attachment 92384 [details] [diff] [review]
context diff patch, sorry for forgetting about the -u

r=brade but I'd prefer to see this line outside of the while loop:
+      nsAutoString value;

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.1, carrying forward r=brade

Comment 12

[kinmoz]

Comment on attachment 96302 [details] [diff] [review]
patch v1.1, carrying forward r=brade

I remember looking at this a while back and wondering why we had to reset
|value| for every iteration of the loop. Does the processValueFunctor() ever
modify |value|? If not it would probably be more efficient to just setup a
|value| and a |valueLowerCase| before the loop and use a pointer to switch
between the two
when necessary right?

In any case, it's not that big of a deal since most of your lookup tables are
only 3+ items long anyways.

sr=[e-mail]

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.2 in answer to kin comments, carryinf forward r=brade and sr=kin

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

checked in (trunk)