635085 - Links in street list of city map of mainz.de do not work. Only the first link works

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[100.59706]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b11) Gecko/20110209 Firefox/4.0b11 SeaMonkey/2.1b2
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b11) Gecko/20110209 Firefox/4.0b11 SeaMonkey/2.1b2

On mainz.de there is a city map (see URL above). By choosing "Straßensuche" you can show all streets beginning with a certain letter. You should be able to move over a specific street to highlight its position in the overview map. If you click on a street name the respective street should be shown in the map. This works only with the very first entry in the list.

Reproducible: Always

Steps to Reproduce:
1. Open http://www.mainz.de/WGAPublisher/online/html/co_stadtplan
2. Click onto "Straßensuche"
3. Chose any initial.
4. Move over the list entries and try to click any.
Actual Results:  
Only the first entry of the list is shown in the map when moved over the street name. As well, only the first street name works as a link to show the respective street in the map.

Expected Results:  
You should be able to show all of the streets whose names are listed.

List works with IE8, FF 3.6 and SM 2.0.

Comment 1

[David Mandelin [:dmandelin]]

Regression range: 

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=83c887dff0da&tochange=d6bb0f9e9519

Now bisecting builds.

Comment 2

[Matthias Versen [:Matti]]

html5.enable to false makes this work.
I bet that this is a bug in the page

This is in the regression range :
Henri Sivonen — Bug 373864 - Enable the HTML5 parser by default. r+sr=jst.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Here's a snippet of markup for the list of street names, with JS stuff
removed for readability.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Screenshot - DOM tree in Fx 4 / 3.6

This is the DOM tree in Fx 4 (left) and Fx 3.6 (right).

In 3.6, when we see the first </a> and have an unclosed <nobr> we just
autoclose it and think nothing more of it.

In 4, it looks like we autoclose it, and then open a new start tag for it
as a sibling of the <a>.  When we see the first <nobr> inside the next
<a> we autoclose the sibling we added (which also closes the <a> which
becomes empty).  The <nobr> and text for that link is outside the intended <a>.

Comment 5

[Henri Sivonen (:hsivonen)]

My wild guess is that bug 605466 has something to do with this.

Since we are so close to shipping, I'm very hesitant to tweak that code anymore. But I guess it's worthwhile to investigate a bit more before sending this over to evang.

Comment 6

[Johnathan Nightingale [:johnath]]

What Henri said. Investigation fine, but this close to shipping I don't think we're going to block on what is likely to be a site bug, or at worst a correctness bug we can fix in a point release.

Comment 7

[Brendan Eich [:brendan]]

Are we parsing per HTML5 and the same as other browsers with HTML5 parsers?

/be

Comment 8

[Mats Palmgren (inactive)]

This is a bug and I have a one-line patch for it.  Details in a few minutes...

Comment 9

[Mats Palmgren (inactive)]

Attachment: home brewed parse trace

http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#adoptionAgency

When we're at the first </a> we have:
 stack: 'nobr''a''body''html'
 listOfActiveFormattingElements: 'nobr''a'
there is no "furthest block" so we close 'nobr' and 'a' and remove
'a' from the list of active formatting elements (but not 'nobr').
we're done with </a> (at 6. in the outer loop)

=> "A start tag whose tag name is one of: "area", "br", "embed", "img", "keygen", "wbr""
When we see <br> we reconstruct formatting elements, a <nobr>.

=> A start tag whose tag name is "nobr"
When we're at the first <nobr> inside the second <a> we have:
 stack: 'a''nobr''body''html'
 listOfActiveFormattingElements: 'a''nobr'
we autoclose the 'a' and 'nobr' and remove 'nobr' from the list of
active formatting elements (but not the 'a').
  * MISSING STEP HERE *
we push the <nobr> and add it to the list of active formatting elements.

The bug is that we should have reconstructed the list of
active formatting elements after autoclosing the <nobr> at
the indicated step above.  That would have inserted the correct <a>.
This is what the spec says (in A start tag whose tag name is "nobr"):
====================
    Reconstruct the active formatting elements, if any.

    If the stack of open elements has a nobr element in scope, then this is a parse error; act as if an end tag with the tag name "nobr" had been seen, then once again reconstruct the active formatting elements, if any.

    Insert an HTML element for the token. Push onto the list of active formatting elements that element.
====================

The part that we're not doing is
"then once again reconstruct the active formatting elements, if any"

Comment 10

[Mats Palmgren (inactive)]

Attachment: fix v1

This works for me locally, let's see what Try says...

Comment 11

[Mats Palmgren (inactive)]

Fwiw, the site works in Google Chrome "10.0.648.82 dev" and Opera 11.00
in addition to UAs mentioned in comment 0.

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 513585 [details] [diff] [review]
fix v1

Passed unit tests on TryServer.
I'll write a regression test for this...

Comment 13

[Henri Sivonen (:hsivonen)]

Comment on attachment 513585 [details] [diff] [review]
fix v1

Good catch.

(FWIW, I saw the difference in Chrome but incorrectly guessed it was due to older loop limits in the AAA.)

Comment 14

[Mats Palmgren (inactive)]

Attachment: mochitest.diff

Comment 15

[Mats Palmgren (inactive)]

Henri, do you think this is worth taking before the release?
Given that the patch is trivial and limited to nested <nobr>'s I don't think
there's much risk of breaking the web.

Comment 16

[Henri Sivonen (:hsivonen)]

Comment on attachment 513585 [details] [diff] [review]
fix v1

> Henri, do you think this is worth taking before the release?

I think it would make sense to take this.

For the test, it would be nice (at least eventually if not for Gecko 2.0) to add the test to html5lib's test suite and then resync the tests from upstream to m-c. I can perform the mechanics if you are OK with that arrangement.

Comment 17

[Mats Palmgren (inactive)]

> I can perform the mechanics if you are OK with that arrangement.

Please do, thanks.

I landed the fix without the test for now.
http://hg.mozilla.org/mozilla-central/rev/e8627a71099c

(FYI, the fix+test did pass on Try)

Comment 18

[Henri Sivonen (:hsivonen)]

Attachment: html5lib patch

Comment 19

[Henri Sivonen (:hsivonen)]

Attachment: htmlparser patch

Comment 20

[Henri Sivonen (:hsivonen)]

Attachment: m-c import of the html5lib patch

Comment 21

[Henri Sivonen (:hsivonen)]

Mats, is it OK if I mark you as the hg patch author of record for the above 3 patches? They are just relocations of your previous patches to this bug.

Comment 22

[Mats Palmgren (inactive)]

Yes

Comment 23

[Henri Sivonen (:hsivonen)]

http://hg.mozilla.org/mozilla-central/rev/3bf94b2e8833
http://hg.mozilla.org/projects/htmlparser/rev/0ba9f2e2c914
http://code.google.com/p/html5lib/source/detail?r=d911de9b18f3ee4769dba633dd47d9a301f96d86