84098 - Link properties dialog doesn't display id

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect, P3)

Description

[basic]

As per HTML 4 section 12.2.3 http://www.w3.org/TR/html4/struct/links.html#h-12.2.3

"The id attribute may be used to create an anchor at the start tag of any
element (including the A element)"

The Link properties dialog should include ids in its list of anchors

Comment 1

[basic]

Attachment: testcase for id attribute

Comment 2

[basic]

Testcase:

1 open testcase http://bugzilla.mozilla.org/showattachment.cgi?attach_id=37129
in composer
2 right click on "this is a link"
3 select "link properties"
4 click on "More Properties"
5 the ids on the page should appear in the named anchor list

Comment 3

[rubydoo123]

moving to 1.0

Comment 4

[Charles Manske]

Wow! Learn something new everyday! This will have to wait until next
version, of course.

Comment 5

[Charles Manske]

This is somewhat complicated to implement:
We would have to grovel through all elements and build a list of IDs.
The should probably be in a separate list, as are the Headings.
Note also:
1. The "name" attribute allows entities, while the id does not.
2. An <a> element with both "id" and "name" must use same string,
3. "Name" and "id" share namespace (no duplicates!)

It also seems like id can simplify the way we create links to headings:
Instead of putting an <a name="foo"></a> before a heading, just add an id="foo" 
to the heading tag directly.

Comment 6

[basic]

how is the current <a name=""> anchor list implemented? Does it use
getElementsByTagName ? Seems like we need something like IE's document.all ;)

id can simplify the way we create links to anything! But do we need to allow <a
name=""> creation for backward compatibility?

Comment 7

[basic]

what about the xul getElementsByAttribute ? Maybe we can use that.

see also the xhtml 1 guidlines http://www.w3.org/TR/html/#guidelines (section
C.8 Fragment Identifiers)

Comment 8

[Charles Manske]

I agree, using ID is really better for creating internal document jumps, but I'm
sure we will have to support standard named anchors for awhile.
Using getElementsByAttribute is a good idea -- I'll look into it.

Comment 9

[Syd Logan]

spam composer change

Comment 10

[Charles Manske]

load balancing

Comment 11

[Charles Manske]

changing milestone

Comment 12

[basic]

Attachment: patch adding ids to named anchor list (uses xpath)

This adds the ids to the named anchor list in the link property dialog

Comment 13

[basic]

Attachment: new patch

here's a new patch. Per request of Glazman, I made the xpath expression select
the named anchors' names as well as IDs. And do a sort before adding them to
the menu popup.

Comment 14

[basic]

ok, I just tested the patch in composer standalone and it does not work. Seems
like composer standalone does not have xpath? Maybe because xslt (transformiix)
is not compiled in?

Comment 15

[basic]

Attachment: new patch non-xpath (uses treeWalker)

Since xpath does not work in composer standalone. Here's a patch that uses
treeWalker instead. I've made some changes to make sure that there are no
duplicates and case insensitive sorting is used.

Comment 16

[basic]

Attachment: new patch non-xpath (uses treeWalker) v2

oops missed a strict warning

Comment 17

[basic]

Attachment: patch v3

oops uploaded the wrong patch. Must be one of those days...

Comment 18

[basic]

Comment on attachment 138159 [details] [diff] [review]
patch v3

glazman suggested I ask Neil for review of this patch

Comment 19

[[e-mail]]

Comment on attachment 138159 [details] [diff] [review]
patch v3

>+    const nodeFilter =
Don't need this.

>+    const treeWalker = editor.document.createTreeWalker(editor.document, 1, nodeFilter, true);
I don't like using const, except for numbers, strings, atoms or interfaces.

+    var anchorMap  = {}; // for weeding out duplicates and making heading
anchors unique
If you use {"": true} then you can save some tests below.

>+    while (element = treeWalker.nextNode())
JS strict warning, sorry

>+      if ((element.tagName.search(/^H[1-6]$/) != -1) &&
As a regexp, the test is best written /^H[1-6]$/.test(element.tagName)/
But neatest is (element instanceof HTMLHeadingElement)

>+          !((child = element.firstChild) && child.nodeName == "A" && child.name.length))
element.firstChild instanceof HTMLAnchorElement && element.name

>+        if (text.length)
if (text) will do.

>+      if(element.tagName == "A")
if (element.firstChild instanceof HTMLAnchorElement)

>+        anchor = '#' + element.name;
>+        if((anchor.length > 1) && !(anchor in anchorMap))
I suggest you don't include the # in the map, then you can use !(element.name
in anchorMap) and merge it into the previous if.

>+      anchor = '#' + element.id;
>+      if((anchor.length > 1) && !(anchor in anchorMap))
Same here, use !(element.id in anchorMap)

>+    delete headingList;
>+    delete anchorMap;
Unnecessary (and probably useless - the syntax is delete x.y or x[y])

>+      for (i = 0; i < anchorList.length; i++)
>+      {
>+        createMenuItem(menupopup,anchorList[i].anchor);
>+      }
It looks as if brace style in this file is to omit braces for single lines.

Comment 20

[basic]

Attachment: patch v4

>>+    const nodeFilter =
>Don't need this.
fixed

>>+    const treeWalker = editor.document.createTreeWalker(editor.document, 1,
nodeFilter, true);
>I don't like using const, except for numbers, strings, atoms or interfaces.
fixed

>+    var anchorMap  = {}; // for weeding out duplicates and making heading
anchors unique
>If you use {"": true} then you can save some tests below.
I'm not sure if it'll help in this case

>>+    while (element = treeWalker.nextNode())
>JS strict warning, sorry
fixed

>>+	 if ((element.tagName.search(/^H[1-6]$/) != -1) &&
>As a regexp, the test is best written /^H[1-6]$/.test(element.tagName)/
>But neatest is (element instanceof HTMLHeadingElement)
fixed

>>+	     !((child = element.firstChild) && child.nodeName == "A" &&
child.name.length))
>element.firstChild instanceof HTMLAnchorElement && element.name
fixed

>>+	   if (text.length)
>if (text) will do.
fixed

>>+	 if(element.tagName == "A")
>if (element.firstChild instanceof HTMLAnchorElement)
fixed

>>+	   anchor = '#' + element.name;
>>+	   if((anchor.length > 1) && !(anchor in anchorMap))
>I suggest you don't include the # in the map, then you can use !(element.name
in anchorMap) and merge it into the previous if.
I think I need the '#' since the anchor name might turn out to be something
like toSource, constructor, prototype, valueOf...

>>+	 anchor = '#' + element.id;
>>+	 if((anchor.length > 1) && !(anchor in anchorMap))
>Same here, use !(element.id in anchorMap)

>>+    delete headingList;
>>+    delete anchorMap;
>Unnecessary (and probably useless - the syntax is delete x.y or x[y])
fixed

>>+	 for (i = 0; i < anchorList.length; i++)
>>+	 {
>>+	   createMenuItem(menupopup,anchorList[i].anchor);
>>+	 }
>It looks as if brace style in this file is to omit braces for single lines.
fixed

Comment 21

[basic]

Attachment: testcase

Comment 22

[basic]

Attachment: patch v5

Comment 23

[[e-mail]]

Comment on attachment 138286 [details] [diff] [review]
patch v4

I don't think that "child" is used any more; your ( and , spacing is
inconsistent; your comment should be "grab named anchors"; otherwise r=me

Comment 24

[basic]

Attachment: patch v6

>I don't think that "child" is used any more; your ( and , spacing is
inconsistent; your comment should be "grab named anchors"; otherwise r=me

1) removed child
2) fixup ( and , spacing
3) add back comment about skipping headings with named anchors as their first
child. Was removed by mistake

should be ready for checkin

Comment 25

[chris hofmann]

Comment on attachment 138321 [details] [diff] [review]
patch v6

1.6-   go ahead and get this on the trunk for 1.7

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 138321 [details] [diff] [review]
patch v6

I'd like neil to restamp.

/be

Comment 27

[[e-mail]]

Comment on attachment 138321 [details] [diff] [review]
patch v6

Thanks to interdiff :-)

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 138321 [details] [diff] [review]
patch v6

>>+      // grab IDs
>+      if (element.id) {

Divergent brace style alert!  Fix that and sr=me.

Cool patch.

/be

Comment 29

[basic]

Attachment: patch v7

(In reply to comment #28)
> (From update of attachment 138321 [details] [diff] [review])
> >>+	   // grab IDs
> >+	  if (element.id) {
> 
> Divergent brace style alert!	Fix that and sr=me.
Fixed

Comment 30

[basic]

Attachment: patch v8

oops, accidentally inserted tabs in the last patch.

Comment 31

[[e-mail]]

Fix checked in.