112276 - Setting "middle" value for "align" attribute on an image is changed to "center"

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[TucsonTester1]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.6+)
Gecko/20011126 Netscape6/6.1b1
BuildID:    20011126

When selecting "In the Center" in image properties -- text alignment, the
setting reverts back to "At the Bottom" when properties are reentered.

Reproducible: Always
Steps to Reproduce:
1. Launch Composer
2. Insert an image
3. Enter some text next to the image
4. Bring up the image properties, and change the text alignment to "In the
Center."  Hit Ok.
5. Go back into the image properties.  

Actual Results:  The setting for "In the Center" changes to "At the Bottom" when
properties are brought back up.

Expected Results:  The setting should remain the same.

None of the other text alignments change to "At the Bottom" when they are
chosen, only "In the Center" does so.

This was also tested and reproduced on 98, 98SE, 95, and XP.

Comment 1

[Syd Logan]

Charles, any idea why this case is different than the others?

Comment 2

[Charles Manske]

Weird! The correct value for the align attribute for IMG element is "middle",
not "center", but somewhere in the code path during setting "middle", it is 
changed to "center"! Thus when you look at the image again, Composer code looks
for "middle", and uses the default "bottom" for the menulist since 
I don't think this problem is in Composer code -- investigating further.
So this is somewhat important since we should be writing the correct values for
HTML attributes.

Comment 3

[Jay Farrell]

I saw this problem on OpenVMS, as well as on Linux. 

Comment 4

[Charles Manske]

According to the HTML spec, the correct value for the (depricated) align attribute
is "middle", not "center". But if I use "getAttribute" to get the "align" value
immediately after setting it by SetAttribute("align", "middle"), it comes back
as "center"

Comment 5

[Johnny Stenback (:jst)]

None of the center attribute handling code has changed in a *long* time from
what I can see, I find it extremely hard to believe that this is a regression,
and since nobody claims that this ever worked in a specific build, I'm removing
the regression keyword.

Also Futuring.

Comment 6

[Charles Manske]

Attachment: Test page: Click on button to view 'align' value after setting it to 'middle'

Comment 7

[Charles Manske]

Yes, it's not a regression. NS 6.x has this problem. 
But futuring? We are writing incorrect HTML.

Comment 8

[Johnny Stenback (:jst)]

Incorrect per the spec maybe, but it still works. We have way bigger fish to fry
for mozilla1.0, if someone has the resources to fix this I'm happy to give
advice on this, but I won't have time to fix it.

Comment 9

[Charles Manske]

Attachment: Fix to Image dialog so "center" can be used as well as "middle"

I realize everyone's busy. We should check this in so Composer's Image
dialog can understand "center"

Comment 10

[Sivakiran Tummala]

bug 85943 is similar to this. 

Comment 11

[Charles Manske]

Attachment: The fix in DOM code for all elements that use align = "middle" instead of "center"

Comment 12

[Charles Manske]

I'll take this back, now that I have a fix

Comment 13

[Charles Manske]

*** Bug 85943 has been marked as a duplicate of this bug. ***

Comment 14

[Charles Manske]

changing milestone. Not worth rushing into 0.9.8

Comment 15

[Sivakiran Tummala]

this happens when setting "bottom" value to align attribute on an image.
bottom is changed to baseline.

Comment 16

[Johnny Stenback (:jst)]

Why aren't you fixing nsGenericHTMLElement::AlignValueToString() to do the
"right" thing if you're sure this is the right thing to do in all these cases?
The current change would introduce maintenance nightmares we don't need in this
code.

Comment 17

[Charles Manske]

Johnny: When we replace nsGenericHTMLElement::EnumTable kAlignTable[] in a 
derived class, you have to call EnumValueToString(aValue, kAlignTable, aResult)
to pick up the enum table in that file. For all elements except the 5 changed 
here, we should use nsGenericHTMLElement::AlignValueToString(), which uses the 
enum table defined in nsGenericElement.cpp (which should use "center", not 
"middle".) An alternate way to fix it is to put a second table with "middle" 
before "center" in nsGenericElement.cpp and test for the 5 element types that 
need that in nsGenericHTMLElement::AlignValueToString() so they use the 2nd table.
Would you prefer that? 
AlignValueToString() seemed kind of superfluous anyway, since it it simply 
called EnumValueToString(aValue, kAlignTable, aResult);

Comment 18

[Johnny Stenback (:jst)]

Yes, I would very much prefere seeing this code only in one place in
nsGenericHTMLElement than duplicated in many different classes.

Comment 19

[Charles Manske]

Sivakiran: Good point about "baseline" vs. "bottom". 
And "top" should be the GetAttribute return value, not "texttop".
The fix for that is similar, by changing locations in the enums:
+static nsGenericHTMLElement::EnumTable kAlignTable[] = {
+  { "left", NS_STYLE_TEXT_ALIGN_LEFT },
+  { "right", NS_STYLE_TEXT_ALIGN_RIGHT },
+  { "top", NS_STYLE_VERTICAL_ALIGN_TOP },//verified
+  { "texttop", NS_STYLE_VERTICAL_ALIGN_TEXT_TOP },// verified
+  { "middle", NS_STYLE_VERTICAL_ALIGN_MIDDLE },//verified
+  { "bottom", NS_STYLE_VERTICAL_ALIGN_BASELINE },//verified
+  { "baseline", NS_STYLE_VERTICAL_ALIGN_BASELINE },// verified
+  { "center", NS_STYLE_VERTICAL_ALIGN_MIDDLE },
+  { "absbottom", NS_STYLE_VERTICAL_ALIGN_BOTTOM },//verified
+  { "abscenter", NS_STYLE_VERTICAL_ALIGN_MIDDLE },/* XXX not the same as ebina */
+  { "absmiddle", NS_STYLE_VERTICAL_ALIGN_MIDDLE },/* XXX ditto */
+  { 0 }
+};
+

Comment 20

[Charles Manske]

Attachment: Fix by adding new method (VAlignValueToString) to nsGenericHTMLElement

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 65258 [details] [diff] [review]
Fix by adding new method (VAlignValueToString) to nsGenericHTMLElement

Much better, sr=jst.

Are we fairly confident that this won't cause regression wrt element alignment?

Comment 22

[Charles Manske]

stummala: do we have some tests to address jst's regression question?

Comment 23

[Kathleen :Brade]

r=brade after regression testing

Comment 24

[Sivakiran Tummala]

yes, we have testcases to test attributes of an elements. 
I will test the build after the check in is done to make sure there are no 
regressions.

Comment 25

[Charles Manske]

let's get this in for 0.9.8 if approved.

Comment 26

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.8

Comment 27

[Charles Manske]

checked in

Comment 28

[michael wendell]

Verified. Looks good to me on Win 2k using trunk build 2002012103.