212165 - Unable to access advanced link properties of a linked image

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[Peto]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; sk-SK; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; sk-SK; rv:1.4) Gecko/20030624

If I want to make the link under image and I want the link to be opened in new
navigator window, I want to use target="_blank" HTML attribute. Usually, it is
possible to add such a thing using Link properties -> Advanced. But if the link
is under image, it is not possible.

Reproducible: Always

Steps to Reproduce:
1.make image
2.make link under the image
3.right-click to get context menu
4.choose Image and link properties
5.press "Advanced" button
6.try to add "target" HTML attribute

Actual Results:  
"target" is not between offered attributes. It seems that only image-related
attributes are avaiable. It seems like no Link HTML attributes is avaiable.
Worse, the same if U use "Link" button.

Expected Results:  
Mozilla should distinguish between Image attributes and Link attributes under
Advanced menu, and it should offer the target attribute for Link.

I cannot say that "there is an easy workaround". It is theoretically possible to
edit the html code manually, but this is not what is Composer dedicated to, and
in more complex page it is really not easy to find the link and add
target="_blank" manually.

Comment 1

[Peto]

Only if I select the whole image (to get it to blue color), then the Link
properties is not mixed with Image properties and it is able to use "target"
attribute.
But if I create Link using the Image menu (what is more intuitive and
comfortable way and I'll expect this from new users), the Link properties are
mixed with image properties in the way I showed.
Maybe there is sub-part of window needed in the Image and Link properties. There
are actually two parts -the main, and the Image advanced (bottom) part with
image preview and "advanced" button. Maybe a third part could take place, with
Link advanced menu and button. Or please try to organise some way. Maybe, if I
switch to Link properties in the Image-Link properties menu, the bottom part
should change from Image advanced to Link advanced, and the Advanced button
should invoke the Link advanced properties instead of Image advanced properties.

Comment 2

[Gérard Talbot]

When an image is embedded inside a link, I think the rigth-click context menu
items could be reorganized in case an user right-clicks on the image only. I
think the Image and Link Properties... menu item should be split in 2 distinct
and separate menu items like:

Image Properties...
-------------------
Link Properties...

where attributes for respective elements can be set, defined via respective
dialog windows. The Image Properties dialog window would still have a Link tab
where the embedding link could be defined, changed, removed.

If the user highlight both the link and image (or just the text link), then only
Link Properties... menu item should be offered like it is right now.

This bug could be resummarized to reflect this proposal. My 2 cents.

XP Pro SP1, Mozilla Composer 1.5 Build 20031007 here.

Comment 3

[Andrew Schultz]

confirmed with linux trunk 20050409

Comment 4

[Ian Neal]

Attachment: Give button for link properties and fix advanced edit value drop down issue

This patch:
* Makes the onChangeHTMLAttribute, onInputHTMLAttributeName and onInputHTMLAttributeValue functions take an argument and change js/xul to use them that way.
* Get onInputHTMLAttributeName to call onInputHTMLAttributeValue with the new value.
* Add onselect triggers to menulists to update the tree.
* Add gLinkElement to EdImageProps and use it for new advanced link editing.
* Add advanced link editing button that calls EdAdvancedEdit.xul but with gLinkElement as an argument instead of globalElement.

Comment 5

[[e-mail]]

Comment on attachment 549674 [details] [diff] [review]
Give button for link properties and fix advanced edit value drop down issue

>diff --git a/editor/ui/dialogs/content/EdAEHTMLAttributes.js b/editor/ui/dialogs/content/EdAEHTMLAttributes.js
You say there's an issue but you don't actually say what the issue is...

>+      if (gAnchorElement)
>+        editor.cloneAttributes(gAnchorElement, gLinkElement);
Does this work in the case of a new link for an existing image?

Comment 6

[Ian Neal]

(In reply to comment #5)
> Comment on attachment 549674 [details] [diff] [review] [diff] [details] [review]
> Give button for link properties and fix advanced edit value drop down issue
> 
> >diff --git a/editor/ui/dialogs/content/EdAEHTMLAttributes.js b/editor/ui/dialogs/content/EdAEHTMLAttributes.js
> You say there's an issue but you don't actually say what the issue is...
This issue is that if you select an attribute which has a fixed list of values, you do not get presented with that list.

> >+      if (gAnchorElement)
> >+        editor.cloneAttributes(gAnchorElement, gLinkElement);
> Does this work in the case of a new link for an existing image?
Yes, it does.

Comment 7

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 549674 [details] [diff] [review]
> > Give button for link properties and fix advanced edit value drop down issue
> > >diff --git a/editor/ui/dialogs/content/EdAEHTMLAttributes.js b/editor/ui/dialogs/content/EdAEHTMLAttributes.js
> > You say there's an issue but you don't actually say what the issue is...
> This issue is that if you select an attribute which has a fixed list of
> values, you do not get presented with that list.
Perhaps that should be a separate patch, so that we can land it on the branch.

> > >+      if (gAnchorElement)
> > >+        editor.cloneAttributes(gAnchorElement, gLinkElement);
> > Does this work in the case of a new link for an existing image?
> Yes, it does.
Steps to reproduce problem:
1. Insert image without link
2. Close the image dialog
3. Edit the newly inserted image
4. Give it a link href
5. Open advanced link properties
6. Give it a blank target
7. Close the dialogs
Expected result: <a target="_blank">
Actual result: Link created but with no target.

Comment 8

[Ian Neal]

Attachment: Just give button for advanced link properties

Changes since last version:
* Spun off value drop down issue into patch on bug 361495
* Fixed issue with new link on existing image (My original testing was setting href on first pass and then other attributes on subsequent passes).

Comment 9

[[e-mail]]

Comment on attachment 549759 [details] [diff] [review]
Just give button for advanced link properties

>+  gDialog.showLinkBorder.disabled = !href;
The Link Properties dialog disables the Advanced Edit button if you have a new link without an href. By comparison the Image Properties dialog disables the Advanced Edit button if you don't have a src. Since we're already enabling the border button based on the href, I guess it would look most consistent if we enabled the Link Advanced Edit button based on the href too.

>+            gAnchorElement = editor.getSelectedElement("href");
Not sure whether you should use
gAnchorElement = editor.getElementOrParentByTagName("href", imageElement);

>         <checkbox id="showLinkBorder"
>                   label="&showImageLinkBorder.label;"
>                   accesskey="&showImageLinkBorder.accessKey;"
>                   oncommand="ToggleShowLinkBorder();"/>
>+        <spacer class="spacer"/>
>+        <hbox>
>+          <spacer flex="1"/>
>+          <button id="LinkAdvancedEditButton"
>+                  label="&LinkAdvancedEditButton.label;"
>+                  accesskey="&LinkAdvancedEditButton.accessKey;"
>+                  tooltiptext="&LinkAdvancedEditButton.tooltip;"
>+                  oncommand="onLinkAdvancedEdit();"/>
>+        </hbox>
I think it would look neater to have the checkbox in the same box, like the relative checkbox and the choose file button.

>+<!ENTITY LinkAdvancedEditButton.accessKey "L">
LinkURLEditField already uses that access key, but maybe it shouldn't.

Comment 10

[Ian Neal]

Attachment: Give a button without a conflicting accesskey [Checked in: Comment 12]

Changes since last version:
* Link Advanced Edit button now disables when href is blank.
* Use getElementOrParentByTagName instead of getSelectedElement for getting gAnchorElement.
* Move button in-line with checkbox.
* Change accesskey for LinkURLEditField to w (which is more visible).

Perhaps the button should be called "Link Properties" or "Advanced Link Properties"?

Comment 11

[[e-mail]]

Comment on attachment 550353 [details] [diff] [review]
Give a button without a conflicting accesskey [Checked in: Comment 12]

No, I'd say Link Advanced Edit makes the most sense.

Comment 12

[Ian Neal]

Comment on attachment 550353 [details] [diff] [review]
Give a button without a conflicting accesskey [Checked in: Comment 12]

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