531540 - Adding images to a page got "dumber" with this release.

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[nicklandsberg]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4) Gecko/20091017 SeaMonkey/2.0
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4) Gecko/20091017 SeaMonkey/2.0

In previous releases, one could add an image, check the "don't use alternate text" button, size the image and add borders, etc.  One can still do this with this release but ... in this release, if one edits the image, all previous information is lost, so one has to check the box again, and sixe the image again, even if only adding a border.  Similarly, if one wants to "resize the image a little bigger" and doens't remember the previous settings (which may have been set in a session sometime ago), onehas to guess at what the previous sizing was and find the right size by trial and error.

Reproducible: Always

Steps to Reproduce:
1. Add an image to a web page, checking off "don't use alt text, set a size, and set a border.
2. Double click on the image to get the dialog back
3. The "don't use alt" boz is unchecked, and the size is the original size rather than the one which had just been specifried.  The border setting remain unchanged, tho.
Actual Results:  
see steps above

Expected Results:  
expected: previous values for check box and size would be retained.

Comment 1

[Ian Neal]

The other issues I believe have been fixed in other bugs.
The "Don't use alt" box is covered in bug 155103
Could you put one issue per bug in future. Thanks.

Comment 2

[[e-mail]]

Actually I think they're the same bug, and I fixed the size in bug 541313, but not the alternate text, which is caused by the same toolkit change. So I'm reopening this bug to fix the "Don't use alternate text" radio not being selected for an existing image without alternate text.

Comment 4

[[e-mail]]

The problem is that the code uses !gDialog.altTextRadioGroup.selectedItem to determine whether the radio group has been initialised yet. Unfortunately changes to toolkit code mean that radio groups now automatically initialise.

Comment 5

[[e-mail]]

Attachment: Proposed patch

I think this might break Thunderbird's "fix" for bug 353205, because it will always force the radio setting for a new image.

Comment 6

[[e-mail]]

Attachment: Better patch

I notice that the previous patch had a bug regarding alt text and Advanced Edit, in that if you took a new image with "Alternate Text" selected but no actual text entered, and you clicked Advanced Edit, it would prefill empty Alternate Text, thus automagically selecting "Don't use alternate text". So I tried to address Thunderbird compatibility at the same time. This does however mean that if you select "Don't use alternate text" and delete the alt attribute in Advanced Edit then "Don't use alternate text" will remain selected.

Comment 7

[Ian Neal]

Comment on attachment 554343 [details] [diff] [review]
Better patch

If you go to insert an image and select the image, the radio button has "Alternative text" selected, going into Advanced Edit shows no alt tag. Switching to "Don't use alternative text" shows an empty alt tag in Advanced Edit. If you remove the alt tag then logically you would expect the radio button to have switched to "Alternative text" but it does not, thus going back into Advanced Edit has an empty alt tag.

Consistent behaviour is needed, so the minimum would be to get alt tag removal in Advanced Edit to switch the radio button to "Alternative text".

My preference is that:
* No alt tag == radio button set to "Don't use alternative text"
* Empty alt tag == radio button set to "Alternative text"

Additionally:
* Inserting new images pre-populates the alt tag with the name of the image minus the file extension (if present).

r- for the moment

Comment 8

[[e-mail]]

(In reply to Ian Neal from comment #7)
> If you go to insert an image and select the image, the radio button has
> "Alternative text" selected, going into Advanced Edit shows no alt tag.
> Switching to "Don't use alternative text" shows an empty alt tag in Advanced
> Edit. If you remove the alt tag then logically you would expect the radio
> button to have switched to "Alternative text" but it does not, thus going
> back into Advanced Edit has an empty alt tag.
I did point out that this was intentional in this version...

Comment 9

[[e-mail]]

(In reply to Ian Neal from comment #7)
> My preference is that:
> * No alt tag == radio button set to "Don't use alternative text"
> * Empty alt tag == radio button set to "Alternative text"
But that's not what happens to the actual element. Empty alt tag must be equivalent to "Don't use alternative text".

> * Inserting new images pre-populates the alt tag with the name of the image
> minus the file extension (if present).
Probably beyond the scope of this bug, but do you mean that when you click "OK" the image name should be extracted from the URL and set as the alt text if no alt text has been specified?

Comment 10

[Ian Neal]

Attachment: alternative approach

I wish thinking of something like this, also doesn't break TB

Comment 11

[[e-mail]]

Attachment: Tweaked patch

OK, so slightly more ugly, but now less likely to be confused by Advanced Edit. However this means that the Thunderbird compatibility now only applies to new images, not existing images that happen to lack an alt attribute.

IanN's patch is wrong because it incorrectly reverses the meaning of the "Don't use alternate text" button with respect to whether the alt attribute is empty or absent.

Comment 12

[Ian Neal]

Comment on attachment 554637 [details] [diff] [review]
Tweaked patch

>+++ b/editor/ui/dialogs/content/EdImageOverlay.js
>@@ -114,43 +114,30 @@ function InitImage()

>   var hasAltText = globalElement.hasAttribute("alt");
>+  var altText = globalElement.getAttribute("alt");
>+  gDialog.altTextInput.value = altText;
>+  if (altText || (!hasAltText && globalElement.hasAttribute("src")))
>   {
>+    gDialog.altTextRadioGroup.selectedItem = gDialog.altTextRadio;
>   }
>+  else if (hasAltText)
>   {
>+    gDialog.altTextRadioGroup.selectedItem = gDialog.noAltTextRadio;
>   }

Wouldn't
+  if (globalElement.hasAttribute("src"))
   {
+    if (altText || !hasAltText)
     {
       gDialog.altTextRadioGroup.selectedItem = gDialog.altTextRadio;
     }
     else
     {
       gDialog.altTextRadioGroup.selectedItem = gDialog.noAltTextRadio;
     }
   }

Work better and not break TB?

Comment 13

[Jonathan Protzenko [:protz]]

Comment on attachment 554637 [details] [diff] [review]
Tweaked patch

STR:
- insert an image with no alt text
- make sure it's not selected
- open the insert image dialog, select "alt text"
- hit cancel,
- open the insert image dialog, note that the "alt text" is still selected,
- hit cancel,
- double-click the previously inserted image,
- hit cancel,
- open the insert image dialog, note that "alt text" is NOT selected, and that "no alt text" is now selected.

I don't think there is a right behavior for that, and given that as far as I can tell this does not break Thunderbird, I think this is ok :). However, I'm asking bwinton for confirmation as this is a potentially sensitive UX issue and we don't want to break the good fix in bug 353205.

For the record, Blake, here's the STR that are fixed by this bug:

- insert an image with no alt text
- make sure it's not selected
- open the insert image dialog, select "alt text"
- hit cancel,
- double-click the previously inserted image,
- note that "alt text" is now selected, instead of "no alt text", which was the existing value for that image.

As this is potentially sensitive, and easily regressed (it's quite a subtle behavior), I'd say we may need a test for that. We already have Mozmill for the composition window, so I'd like to see a test that prevents my second STR from being broken.

Comment 14

[Jonathan Protzenko [:protz]]

The more I think about it, the more I think that double-clicking on an image should not affect the previously remembered value for the dialog when opened through the insert image action. Would IanN's suggestion actually implement that?

Comment 15

[[e-mail]]

(In reply to Jonathan Protzenko from comment #14)
> The more I think about it, the more I think that double-clicking on an image
> should not affect the previously remembered value for the dialog when opened
> through the insert image action.
Your problem is that the use of persist means that the dialog (for a new image, or without this patch, for an image without alternate text) opens with whatever setting was chosen when it was last closed, even if it was cancelled. If you want anything more complicated then you would need to write code for it.

Comment 16

[Jonathan Protzenko [:protz]]

Neil, can't you  just unset dynamically the remember=selected attribute when opening the dialog to edit the properties of an existing image, prior to making sure the right value is selected? That would make sure we persist the selection only when opening it for a new image, wouldn't it?

Comment 17

[[e-mail]]

(In reply to Jonathan Protzenko from comment #16)
> Neil, can't you just unset dynamically the remember=selected attribute when
> opening the dialog to edit the properties of an existing image, prior to
> making sure the right value is selected? That would make sure we persist the
> selection only when opening it for a new image, wouldn't it?

I could do. I guess since I've already got some code to work around Thunderbird, a little more wouldn't hurt.

Comment 18

[Jonathan Protzenko [:protz]]

Great, thanks!

Comment 19

[[e-mail]]

Oops, I just realised that it isn't as easy as I thought - this code is also used to process the result of the Advanced Edit dialog, so that would erroneously trigger the "existing image" code path in that case.

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 554637 [details] [diff] [review]
Tweaked patch

Review of attachment 554637 [details] [diff] [review]:
-----------------------------------------------------------------

So, we're definitely going to need a test since it's fairly strange behaviour, so I'm going to mark this r-, but the code is generally okay.

I agree with Protz that we really would like the alt text setting to only be remembered for new image insertions.
Are you planning on submitting a new patch that implements that?

Thanks,
Blake.

::: editor/ui/dialogs/content/EdImageOverlay.js
@@ -125,3 +124,3 @@
> >    var hasAltText = globalElement.hasAttribute("alt");
> > -  var altText;
> > +  var altText = globalElement.getAttribute("alt");
> > -  if (hasAltText)
> > +  gDialog.altTextInput.value = altText;

Could we just check for altText, instead of checking hasAltText?
And since we're always setting gDialog.altTextInput.value, do we need the altText variable?

@@ +132,1 @@
>    {

Since these are now all one-liners, we can remove the {s and }s.

Comment 21

[[e-mail]]

(In reply to Blake Winton from comment #20)
> So, we're definitely going to need a test since it's fairly strange behaviour
Except we can't since the behaviour is different on Thunderbird and SeaMonkey due to bug 353205.

> Are you planning on submitting a new patch that implements that?
This bug is about fixing the regression caused by bug 371260. I have tried to avoid regressing bug 353205 as much as I can. As per comment 19 there's not much more I can do. If you need any new behaviour you should file your own followup to bug 353205.

> Could we just check for altText, instead of checking hasAltText?
No. Empty alt text != no alt text.

> And since we're always setting gDialog.altTextInput.value, do we need the
> altText variable?
It's more work to set and get the value from the textbox, but I will change it if IanN agrees with you.

> Since these are now all one-liners, we can remove the {s and }s.
Sure.

Comment 22

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 554637 [details] [diff] [review]
Tweaked patch

(In reply to [e-mail] from comment #21)
> (In reply to Blake Winton from comment #20)
> > So, we're definitely going to need a test since it's fairly strange behaviour
> Except we can't since the behaviour is different on Thunderbird and
> SeaMonkey due to bug 353205.

Well, we could write a mozmill test, which I believe SeaMonkey doesn't run…  (Although I may be wrong about that.)

> > Are you planning on submitting a new patch that implements that?
> This bug is about fixing the regression caused by bug 371260. I have tried
> to avoid regressing bug 353205 as much as I can. As per comment 19 there's
> not much more I can do. If you need any new behaviour you should file your
> own followup to bug 353205.

Yeah, I guess it would make sense to put the tests in the followup bug.  Protz, did you want to take a stab at it?

> > And since we're always setting gDialog.altTextInput.value, do we need the
> > altText variable?
> It's more work to set and get the value from the textbox, but I will change
> it if IanN agrees with you.

Cool, thanks.

So, given the changes you made, and the likelyhood of a followup bug to fix the other bits, I'll change my mind, and say r=me for the next version of this patch.

Thanks,
Blake.

Comment 23

[Ian Neal]

Comment on attachment 554637 [details] [diff] [review]
Tweaked patch

Steps to reproduce:
* Start composer
* Add an image and set "Don't use alternate text"
* Advanced Edit shows an empty alt entry
* OK dialog
* Open image properties
* Alternative Text radio is selected
* Advanced Edit shows an empty alt entry
* Remove alt entry in Advanced Edit
* Go back into Advanced Edit - empty alt entry still there

r- for the moment

Comment 24

[[e-mail]]

(In reply to Ian Neal from comment #23)
> Steps to reproduce:
> * Start composer
> * Add an image and set "Don't use alternate text"
> * Advanced Edit shows an empty alt entry
> * OK dialog
> * Open image properties
> * Alternative Text radio is selected
I don't see that here in my patched build.

> * Advanced Edit shows an empty alt entry
> * Remove alt entry in Advanced Edit
> * Go back into Advanced Edit - empty alt entry still there
I don't see that either.

Comment 25

[Ian Neal]

Comment on attachment 554637 [details] [diff] [review]
Tweaked patch

(In reply to [e-mail] from comment #24)
> (In reply to Ian Neal from comment #23)
> > Steps to reproduce:
> > * Start composer
> > * Add an image and set "Don't use alternate text"
> > * Advanced Edit shows an empty alt entry
> > * OK dialog
> > * Open image properties
> > * Alternative Text radio is selected
> I don't see that here in my patched build.
> 
> > * Advanced Edit shows an empty alt entry
> > * Remove alt entry in Advanced Edit
> > * Go back into Advanced Edit - empty alt entry still there
> I don't see that either.

Clobber build fixed it.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #22)
> Comment on attachment 554637 [details] [diff] [review]
> Tweaked patch
> 
> (In reply to [e-mail] from comment #21)
> > (In reply to Blake Winton from comment #20)
> > > And since we're always setting gDialog.altTextInput.value, do we need the
> > > altText variable?
> > It's more work to set and get the value from the textbox, but I will change
> > it if IanN agrees with you.
I don't see any advantage of doing that, so stick with it as it is in the current patch.

r=me as long as you look at whether my suggestion on enclosing the radiogroup selection within an if (globalElement.hasAttribute("src")) still lets the patch work but doesn't regress things for TB.

Comment 26

[Jonathan Protzenko [:protz]]

Attachment: Patch with tests

Here's a test.

Comment 27

[Jonathan Protzenko [:protz]]

Attachment: Slightly better version of the test

Comment 28

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 557228 [details] [diff] [review]
Slightly better version of the test

Review of attachment 557228 [details] [diff] [review]:
-----------------------------------------------------------------

It would be nice to have another test for the behaviour of the option when we open and close the image properties, possibly changing the "Don't use alternate text" setting.

But that doesn't block the r=me, with the nits below fixed.

(Note: I've tested this only on Windows.  It might be a good idea to attach a link to a try-server run to see if things also work on Mac and Linux.)

Thanks,
Blake.

::: mail/test/mozmill/composition/test-dialogs.js
@@ +13,5 @@
> + *
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + * Mozilla Messaging.

This should be "The Mozilla Foundation".  (I can't find where I heard that from, but I think it was Gerv...)
Also, "Mozilla Messaging" doesn't really exist anymore.  :)

Comment 29

[Jonathan Protzenko [:protz]]

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=af70cf70a47c

I'll check that in as soon as I'm sure the tests are green. I added an extra test for the case you mention.

Comment 30

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/comm-central/rev/e3f246082889

Test is green on OSX, pushed!