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

NEW
Assigned to

Status

()

defect
--
major
17 years ago
8 months ago

People

(Reporter: dbaron, Assigned: emilio, NeedInfo)

Tracking

(Blocks 5 bugs)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [whitebox])

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

17 years ago
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

17 years ago
 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

Updated

17 years ago
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 → ---

Comment 4

11 years ago
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.

Comment 11

9 years ago
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
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

Updated

9 months ago
Product: Core → Core Graveyard

Updated

9 months ago
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.