135040 - fix imagemaps so they don't misuse the primary frame mapping

Status: NEW

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

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

This is a bug to fix imagemaps so they don't misuse the primary frame map.  Once
this is done we can shrink the primary frame map hashtable entries back down to
2 words.  See bug 115481.

Comment 1

[Aaron Leventhal]

For accessibility bug 132158, we need to be able to report the bounding
rectangle of image map areas.

To do this, we need to be able to GetPrimaryFrameFor an image map area. When we
ask the frame for it's bounds, we need to get the correct bounds for the area,
not the whole map.

Is this at all related?

Comment 2

[Gilbert Fang]

 Hi, aaron
In my option , the image map 's primary frame should be the image frame which
use it. It is the FindPrimaryFrameFor function which in a given condition can
not find the correct Primary frame for that   image map. If in bug 132158 , the
primary frame is *correct* then the two bugs are *no* direct relationship,
otherwise, GetPrimaryFramFor() will return a nsnull frame.
That is why  in bug 124789 any key press does not work, because we lose the
frame to handle that key event. 
The frame and the content is not a one by one mapping actually , so finding 
primary frame according to content will casue strange errors. see bug 135083.
I am not sure  when should we use primary frame map, in this case do I misuse it?  

Comment 3

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

I think this is, to some degree, related to those issues.  Perhaps the solution
is to make the area objects inherit from nsFrame?  (Might it be possible to make
the focus code less hacky that way?)

Comment 4

[Jesse Ruderman]

Nominating for blocking1.9.1 because this blocks an sg:critical bug.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Even if area objects inherit from nsFrame, the bug 132158 thing will still be broken if the same map is used by multiple images on the page...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Once this is done, please search the source for all the XXX comments about working around this bug and remove the workarounds.

Comment 7

[Timothy Nikkel (:tnikkel)]

Worth fixing at the same time as this bug would be to make sure image maps don't setup during painting. This should be done at the very least when layout is flushed, not painting. The test for bug 517737 has to work around this by trying to force a paint.

Comment 8

[Zack Weinberg (:zwol)]

Didn't we kill off the primary frame map?

Comment 9

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

Well, there's still a mapping from content to primary frame:  it's just a pointer rather than an out-of-band hashtable.  And imagemaps still misuse it.

Comment 10

[Timothy Nikkel (:tnikkel)]

Another thing that needs fixing: if you remove an area element from the DOM then the document updates correctly. But if you remove the map element it's like nothing happened.

Comment 11

[Jesse Ruderman]

This bug keeps making me more and more sad.
http://mxr.mozilla.org/mozilla-central/search?string=135040

Comment 12

[Zack Weinberg (:zwol)]

Dumb question time: What *relies* on the hack being in place?  It is not obvious to me from reading through nsImageMap.cpp.

Comment 13

[Timothy Nikkel (:tnikkel)]

That's a good question, and one that will need to be answered to fix this bug. A quick look I found these function depending on it:

nsImageMap::GetBoundsForAreaContent
nsImageMap::ChangeFocus
nsImageFrame::GetContentForEvent
nsImageFrame::HandleEvent

The hard part will be finding if there are any other more subtle uses, I don't think our testsuite has good coverage of image maps.

Comment 14

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

(In reply to comment #12)
> Dumb question time: What *relies* on the hack being in place?  It is not
> obvious to me from reading through nsImageMap.cpp.

Last I checked, focus (e.g., tabbing through an imagemap).

That said, I don't think focus works if two images use the same imagemap, which is perfectly legal.  (It might work for the first... or maybe it's the last?)

Comment 15

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

(In other words, the last time I tried fixing this I found everything relatively straightforward except for focus.  But the focus code is a lot cleaner now, so things are probably better.)

Comment 16

[Timothy Nikkel (:tnikkel)]

Some code I noticed that might need attention when fixing this bug: nsFocusManager::CheckIfFocusable http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1365

Comment 17

[Emilio Cobos Álvarez (:emilio)]

I was going to hack around this for stylo again, but I think I came up with a fix that does work, so I'll post some patches here instead.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Current attempt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae74ef43d9d3d703c57109af481e6232c2802dbf

Comment 19

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=824485c62785ee1408b5b5f4dfcdda20314062a8

Comment 20

[Emilio Cobos Álvarez (:emilio)]

So, I've been intermittently trying to get this into a landable state, and it's been a huge pain so far...

There's a lot of code under dom/events and accessible/ that rely on this. And that code is surprisingly very well tested.

My approach was introducing a GetEventFrame() that was basically the equivalent to what is GetPrimaryFrame() now, with an out-of-line path for area elements, and use it in the relevant places... But the relevant places end up being a ton.

We've fixed restyling of imagemaps properly in the meantime, so I don't think I have a particular reason to pursue this right now. Happy to share the patches I have so far, I can probably land a few of the preliminary cleanups in a separate bug.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Classic, the bug I didn't manage to hunt down was in the "preliminary cleanup"...

I'll keep working on this on my free time. I feel really sad when I see all the references to this bug.

Comment 22

[Mats Palmgren (inactive)]

Thanks for working on this Emilio.  I would very much like to see this bug fixed;
let me know if I can help in any way.  FYI, I had to add yet another workaround
for it in bug 1400618.

I think this also blocks bug 1412568, and we should probably take that into
account in this bug.  That is, an <area> *may* have its own primary frame.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #22)
> Thanks for working on this Emilio.  I would very much like to see this bug
> fixed;
> let me know if I can help in any way.  FYI, I had to add yet another
> workaround
> for it in bug 1400618.
> 
> I think this also blocks bug 1412568, and we should probably take that into
> account in this bug.  That is, an <area> *may* have its own primary frame.

Right, I can post the wip patches I have if you want. I'm not actively working on this right now, but those were pretty close to green.

The approach basically was to store the image frame as a node property on the <area> element, and adapt the event handling methods to reach it when necessary. AIUI that'd allow <area> to have their own primary frame.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

ni? Me to rebase, and at least put some patches here. Maybe I can figure out the failures too.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Here are the rebased patches. Looks pretty close and I suspect that some of the failures are because of our support of <a> in image maps.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b9debd70ad05449ae69356db973366321f4f30f&selectedJob=141767806

I can try to look at this if I find the time, but if someone wants to help out and investigate, that'd be helpful too.

Comment 26

[Mats Palmgren (inactive)]

Sounds like we should just land bug 1317937 then...
It's blocked on getting telemetry about web usage, but that never
happened (bug 1350532).  If bug 1317937 comment 4 is right that
Firefox is the only browser supporting "<a> as <area>" then I think
it's very safe to remove it without getting telemetry first.

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)

Here are the rebased patches. Looks pretty close and I suspect that some of
the failures are because of our support of <a> in image maps.
[...]
(In reply to Mats Palmgren (inactive) from comment #26)
Sounds like we should just land bug 1317937 then...

Note, it looks like we did land bug 1317937 shortly after these comments^ (disallowing <a> as <area> in image maps), so maybe someone could proceed from where emilio left off in comment 25 when they find the time.

To my surprise, the try commits from comment 25 (with emilio's rebased patches at that point) still exist, too -- hooray!

(I think Try gets purged periodically -- or at least it used to -- so it's not a long-term-store-of-patches. But it seems that hasn't happened in the last 5 years, I guess, so the patches are still there.)

Comment 28

[Daniel Holbert [:dholbert]]

Attachment: emilio's part 1 from try run in comment 25

Just posting the Try run patches from comment 25, for archival purposes / future-use, so they're not lost if Try gets purged (as IIRC it sometimes does).

Comment 29

[Daniel Holbert [:dholbert]]

Attachment: emilio's part 2 from try run in comment 25