371195 - odd "left" and "right" arrow images in the "Previous" / "Next" buttons in the table properties dialog

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[(not reading, please use [e-mail] instead)]

odd "left" and "right" arrow images in the "Previous" / "Next" buttons in the table properties dialog

here comes a screen shot.

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: screen shot

Comment 2

[Scott MacGregor]

It's super easy to remove these styles from the two buttons by altering editorDialog.css. I agree they look weird. It looks like they were intentionally put on those two buttons back before the dawn of time though and were added intentionally. I think the composer dialog is using those two buttons to let you navigate the underlying table and they use the arrows to indicate to you which way the selection is going to move if you push the button. The icons even change if you have a row selected vs. a cell.

Not sure what the right thing to do is.

Comment 3

[(not reading, please use [e-mail] instead)]

> The icons even change if you have a row selected vs. a cell.

wow, you are right.  Do you see a down arrow on the "next" button when you do "row"?  I don't.  Do you want a spin off bug on that?

> Not sure what the right thing to do is.

me neither.  but at the very least, maybe add some padding to separate the image from the text in the button?

Comment 4

[Phil Ringnalda (:philor)]

(In reply to comment #3)
> wow, you are right.  Do you see a down arrow on the "next" button when you do
> "row"?  I don't.  Do you want a spin off bug on that?

Filed bug 406416 - just a Hewitt typo from 2001.

> > Not sure what the right thing to do is.

I suspect the right thing is to narrow it down to a bug in the Windows native theming - the arrows aren't smashed against the text in Mac or Linux trunk, Tb or SeaMonkey, but I'm betting if you put a list-style-image on a button in Fx, you'll wind up with the same thing.

Comment 5

[Wayne Mery (:wsmwk)]

I still see this
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.5) Gecko/20091215 Shredder/3.0.1pre

Comment 6

[:aceman]

Still valid on TB19.
So is it a problem with the icon in Windows theme or a toolkit bug of wrongly rendering the list-style-image (not enough padding)?

Comment 7

[[e-mail]]

Attachment: old classic theme style

This gives you a rough approximation of what the Classic theme used to look like in the Mozilla suite. As you can see the arrow images contained some built-in padding (I believe they were basically 11x11 transparent squares with the arrow drawn in black).

At some point the arrow images were cropped to the minimum size, however presumably nobody bothered to check non-Firefox users...

Comment 8

[[e-mail]]

(In reply to comment #7)
> At some point the arrow images were cropped to the minimum size
And the arrows themselves were reduced in size too it seems.

Comment 9

[:aceman]

Maybe it is just weird to apply list-style-image onto a button element. Can we rewrite it as <button><image/><label></button> ?

Comment 10

[:aceman]

Or even <button image=""> ?

Comment 11

[[e-mail]]

(In reply to aceman from comment #9)
> Maybe it is just weird to apply list-style-image onto a button element.
It's an inheritable CSS property, so the image inherits it from the button. We do that with most of our images, although sometimes people forget and write less efficient CSS that targets the image directly instead.

> Can we rewrite it as <button><image/><label></button> ?
We could do, and that would let us set a class on the image to give it padding, while still inheriting the image from the button. (If you don't want to inherit the image from the button then you have to fiddle about getting the script to update the attribute on the image instead of the button.)

(I would suggest doing the MoreFewerButton while you're there but it turns out it's only used in hidden dialogs - you would have to manually insert a form control before you could open the dialog in question.)

(In reply to aceman from comment #10)
> Or even <button image=""> ?
That would make no sense at all.

Comment 12

[:aceman]

(In reply to [e-mail] from comment #11)
> (In reply to aceman from comment #10)
> > Or even <button image=""> ?
> That would make no sense at all.

Is <button image="..." label="..."> not rendered they way we want it here? I'll check it out.

Comment 13

[[e-mail]]

Setting the image through the attribute achieves the same result as setting it through CSS except that you fix the location of the image (setting it in CSS only fixes the location of the stylesheet, but themes expect that).

Comment 14

[Richard Marti (:Paenglab)]

It could also be possible to add the old arrow icons in qute and point in CSS to them instead to global.

Comment 15

[:aceman]

It seems rewriting EdTableProps.xul to:
<button id="PreviousButton" accesskey="&cellSelectPrevious.accessKey;" oncommand="MoveSelection(0)" flex="1"><image/><label value="&cellSelectPrevious.label;"/></button>
<button id="NextButton" class="align-right"  accesskey="&cellSelectNext.accessKey;" oncommand="MoveSelection(1)" flex="1"><image/><label value="&cellSelectNext.label;"/></button>

seems to work without any changes to css.

Only that the accesskey is not working yet, nor on <label>. I'll need to find out where to put it.

Comment 16

[[e-mail]]

(In reply to aceman from comment #15)
> It seems rewriting EdTableProps.xul to:
> <button id="PreviousButton" accesskey="&cellSelectPrevious.accessKey;"
> oncommand="MoveSelection(0)" flex="1"><image/><label
> value="&cellSelectPrevious.label;"/></button>
> <button id="NextButton" class="align-right" 
> accesskey="&cellSelectNext.accessKey;" oncommand="MoveSelection(1)"
> flex="1"><image/><label value="&cellSelectNext.label;"/></button>
> 
> seems to work without any changes to css.
> 
> Only that the accesskey is not working yet, nor on <label>. I'll need to
> find out where to put it.

[Wild guess] Try:
<button id="PreviousButton" oncommand="MoveSelection(0)" flex="1"><image/><label value="&cellSelectPrevious.label;" accesskey="&cellSelectPrevious.accessKey;" control="PreviousButton"/></button>

Comment 17

[:aceman]

Yes, found that out too.
This seems to work and render fine, on Win XP:

<button id="PreviousButton" oncommand="MoveSelection(0)" flex="1"><image/><label value="&cellSelectPrevious.label;" accesskey="&cellSelectPrevious.accessKey;" control="PreviousButton"/></button>
<button id="NextButton" class="align-right" oncommand="MoveSelection(1)" flex="1"><image/><label value="&cellSelectNext.label;" accesskey="&cellSelectNext.accessKey;" control="NextButton"/></button>

Even the changing to "row" changes the icons fine.

Comment 18

[:aceman]

Neil, would you be OK with this solution? The patch would consist only of these 2 lines. Or do I need to do anything else? You mentioned some MoreFewerButton but that is in a different file so I do not understand what to do with that and how it is related.

Comment 19

[[e-mail]]

(In reply to aceman from comment #18)
> Neil, would you be OK with this solution? The patch would consist only of
> these 2 lines. Or do I need to do anything else? You mentioned some
> MoreFewerButton but that is in a different file so I do not understand what
> to do with that and how it is related.

Sure, we'll take a look at this solution, and then I can always look into porting the changes to work with the MoreFewerButton.

Comment 20

[:aceman]

Attachment: patch

Comment 21

[[e-mail]]

Comment on attachment 676273 [details] [diff] [review]
patch

I guess this is about as good as we can do without replacement arrow images.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/95500eb42b5f

Comment 23

[Mark Banner (:standard8)]

I believe this isn't specific to SeaMonkey, but affects Thunderbird as well in the compose window (the properties dialog is there)

Comment 24

[[e-mail]]

Mark Banner changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|Composer                    |Composition
            Product|SeaMonkey                   |MailNews Core

I admit I'm not sure what the best fit for editor/ui/dialogs/content is.

Comment 25

[[e-mail]]

I guess I should have done that as some sort of quote but you can't easily quote field changes in Bugzilla ;-)