342145 - "ASSERTION: Previous Sibling is the Container's frame" involving image map

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P4)

Description

[Jesse Ruderman]

###!!! ASSERTION: Previous Sibling is the Container's frame: 'prevSibling != containerFrame', file /Users/admin/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8651

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The problem is that the <area> has the ImageFrame as its primary frame,
which is also the primary frame for the <img> which is its content parent.

Comment 3

[Mats Palmgren (inactive)]

An alternative would be verify that "prevSibling->GetContent()" 
is actually the content we are looking at (*iter), here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1237&root=/cvsroot&mark=8619#8601
But I think I prefer the attached patch... Boris?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm... This patch seems to make sense, but I'd want David to give it a look too...

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 226438 [details] [diff] [review]
Patch rev. 1

See comment 2 - 4, what do you think?

Comment 6

[Jesse Ruderman]

dbaron, ping?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I've forgotten why we set primary frame for areas -- but whatever it is, it's broken, because multiple images can refer to the same image map.  How hard is that to fix?  I've forgotten.

(That's also the sole reason why we bloat the primary frame map entries by a word, unless somebody's fixed that...)

Comment 8

[Boris Zbarsky [:bzbarsky]]

> So I've forgotten why we set primary frame for areas

I thought it had to do with tabbing....  we want to be able to tab between the <area>s within an image, and I thought the ESM needed a frame for that.  Mats or Aaron might know?

Comment 9

[Aaron Leventhal]

Boris, the code and comments seem to disagree, and it's been a while since I've looked at that closely. But, look at this:
http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#3587
3585       if (currentContent->Tag() == nsGkAtoms::img &&
3586           currentContent->HasAttr(kNameSpaceID_None, nsGkAtoms::usemap)) {
3587         // Must be an image w/ a map -- it's tabbable but no tabindex is specified
3588         // Special case for image maps: they don't get walked by nsIFrameTraversal
3589         nsIContent *areaContent = GetNextTabbableMapArea(forward, currentContent);
3590         if (areaContent) {
3591           NS_ADDREF(*aResultNode = areaContent);
3592           return NS_OK;
3593         }
3594       }

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, cool.  If it just works on content, maybe we don't need this anymore...

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So that means we can fix this by undoing http://bonsai.mozilla.org/cvslog.cgi?file=/mozilla/layout/base/nsFrameManager.cpp&rev=HEAD&mark=1.100#1.100 and not making the area's primary frame the frame for the first image to use that area?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 226438 [details] [diff] [review]
Patch rev. 1

In any case, this patch doesn't seem right.  If we want a quick fix here, I think it would be to explicitly exclude imagemap areas (which are sometimes A elements!).

But I suspect there are bigger problems caused by what we're doing here (e.g., what happens if the area is an A element that's also displayed elsewhere, or when multiple images use the same areas).  So I think the real solution is to stop setting the primary frame for imagemap areas and fix the tabbing code to deal.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> So that means we can fix this by undoing ...

Possibly, yes.  Someone needs to check that tabbing through multiple images that use the same imagemap (e.g. on slashdot) works after that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Er, doesn't look like slashdot does that anymore.  So we'd need a testcase too.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I tried the testcase in the original bug about the problem, bug 115481, and it doesn't work.  But I think we need to make it work.

Comment 16

[Jesse Ruderman]

Mats, are you going to be able to fix this for Gecko 1.9?  If not, please reassign to nobody so we know this bug needs a new owner.  (It's blocking1.9+.)

Comment 17

[Bob Clary [:bc] (inactive)]

Attachment: testcase - multiple images using the same map

Comment 18

[Jesse Ruderman]

Attachment: testcase

Moved the image into the testcase file, reduced the timeout.

I can still reproduce the bug on trunk.

Comment 19

[Jesse Ruderman]

WFM on trunk.

Comment 20

[Jesse Ruderman]

Crashtest: http://hg.mozilla.org/mozilla-central/rev/b64a7e4a35e2