Open Bug 135040 Opened 23 years ago Updated 1 year ago

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

Categories

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

Product:
Core
Component:
Layout: Images, Video, and HTML Frames
Type:
defect

Tracking

()

People

(Reporter: dbaron, Assigned: emilio, NeedInfo)

References

(Blocks 5 open bugs)

Details

(Whiteboard: [whitebox])

Attachments

(2 files)

emilio's part 1 from try run in comment 25
(deleted), text/plain
Details
emilio's part 2 from try run in comment 25
(deleted), text/plain
Details
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.
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?
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?
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?)
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Future
Whiteboard: [whitebox]
Assignee: dbaron → nobody
Severity: normal → major
Status: ASSIGNED → NEW
Component: Layout → Layout: Images
Priority: P2 → --
QA Contact: chrispetersen → layout.images
Target Milestone: Future → ---
Blocks: 135083, 342145
Blocks: 386449
Blocks: 386450
Blocks: 437142
Nominating for blocking1.9.1 because this blocks an sg:critical bug.
Flags: blocking1.9.1?
Flags: blocking1.9.1? → wanted1.9.1+
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...
Once this is done, please search the source for all the XXX comments about working around this bug and remove the workarounds.
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.
Didn't we kill off the primary frame map?
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.
Summary: fix imagemaps so they don't misuse the primary frame map → fix imagemaps so they don't misuse the primary frame mapping
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.
This bug keeps making me more and more sad. http://mxr.mozilla.org/mozilla-central/search?string=135040
Dumb question time: What *relies* on the hack being in place? It is not obvious to me from reading through nsImageMap.cpp.
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.
(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?)
(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.)
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
Blocks: 616333
Blocks: 725921
Blocks: 749326
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.
Assignee: nobody → emilio+bugs
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.
Assignee: emilio+bugs → nobody
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.
Assignee: nobody → emilio+bugs
Depends on: 1382593
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.
Blocks: 1412568
(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.
ni? Me to rebase, and at least put some patches here. Maybe I can figure out the failures too.
Flags: needinfo?(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.
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.
No longer blocks: 1317937
Depends on: 1317937
Product: Core → Core Graveyard
Product: Core Graveyard → Core
Depends on: 1745860
No longer depends on: 1745860
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1745860

(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.)

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).

Severity: major → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: