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

VERIFIED FIXED in mozilla0.9.8

Status

()

Core
DOM: Core & HTML
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: TucsonTester1, Assigned: Charles Manske)

Tracking

Trunk
mozilla0.9.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: FIX IN HAND, have reviews; need regression testing)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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

16 years ago
Charles, any idea why this case is different than the others?
Assignee: syd → cmanske
(Assignee)

Comment 2

16 years ago
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.
Severity: minor → normal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: regression
Target Milestone: --- → mozilla0.9.8

Comment 3

16 years ago
I saw this problem on OpenVMS, as well as on Linux. 
(Assignee)

Comment 4

16 years ago
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"
Assignee: cmanske → jst
Status: ASSIGNED → NEW
Component: Editor: Composer → DOM HTML
QA Contact: sujay → stummala
Summary: Align text changes to "At the Bottom" when "In the Center" is selected. → Setting "middle" value for "align" attribute on an image is changed to "center"
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.
Keywords: regression
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: mozilla0.9.8 → Future
(Assignee)

Comment 6

16 years ago
Created attachment 65071 [details]
Test page: Click on button to view 'align' value after setting it to 'middle'
(Assignee)

Comment 7

16 years ago
Yes, it's not a regression. NS 6.x has this problem. 
But futuring? We are writing incorrect HTML.
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.
(Assignee)

Comment 9

16 years ago
Created attachment 65078 [details] [diff] [review]
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

16 years ago
bug 85943 is similar to this. 
(Assignee)

Comment 11

16 years ago
Created attachment 65131 [details] [diff] [review]
The fix in DOM code for all elements that use align = "middle" instead of "center"
Attachment #65078 - Attachment is obsolete: true
(Assignee)

Comment 12

16 years ago
I'll take this back, now that I have a fix
Assignee: jst → cmanske
Whiteboard: FIX IN HAND, need r=,sr=
Target Milestone: Future → mozilla0.9.8
(Assignee)

Comment 13

16 years ago
*** Bug 85943 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 14

16 years ago
changing milestone. Not worth rushing into 0.9.8
Status: NEW → ASSIGNED
Keywords: patch, review
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Comment 15

16 years ago
this happens when setting "bottom" value to align attribute on an image.
bottom is changed to baseline.
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.
(Assignee)

Comment 17

16 years ago
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);
Yes, I would very much prefere seeing this code only in one place in
nsGenericHTMLElement than duplicated in many different classes.
(Assignee)

Comment 19

16 years ago
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 }
+};
+

(Assignee)

Comment 20

16 years ago
Created attachment 65258 [details] [diff] [review]
Fix by adding new method (VAlignValueToString) to nsGenericHTMLElement
Attachment #65131 - Attachment is obsolete: true
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?
Attachment #65258 - Flags: superreview+
(Assignee)

Comment 22

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

Comment 23

16 years ago
r=brade after regression testing
Whiteboard: FIX IN HAND, need r=,sr= → FIX IN HAND, have reviews; need regression testing

Comment 24

16 years ago
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.
(Assignee)

Comment 25

16 years ago
let's get this in for 0.9.8 if approved.
Target Milestone: mozilla0.9.9 → mozilla0.9.8
a=blizzard on behalf of drivers for 0.9.8
Keywords: mozilla0.9.8+
(Assignee)

Comment 27

16 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 28

16 years ago
Verified. Looks good to me on Win 2k using trunk build 2002012103.
Status: RESOLVED → VERIFIED

Updated

9 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.