Last Comment Bug 698228 - [ImageMap] Don't use GetPrimaryFrame when not needed
: [ImageMap] Don't use GetPrimaryFrame when not needed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Images (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-29 15:28 PDT by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2011-11-27 23:08 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.34 KB, patch)
2011-10-29 15:28 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
mats: review+
surkov.alexander: feedback+
Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-29 15:28:51 PDT
Created attachment 570504 [details] [diff] [review]
patch

Since we have the strange setup that area elements get the primary frame of
img, there is really no need to use GetPrimaryFrame in nsImageMap.
Especially because we should get rid of that strange setup.
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-29 15:31:14 PDT
Comment on attachment 570504 [details] [diff] [review]
patch

surkov, could you test that a11y still works ok.
It is the only using GetBoundsForAreaContent, IIRC.
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-29 15:31:38 PDT
https://tbpl.mozilla.org/?tree=Try&rev=aca8dcfb5f49
Comment 3 Mats Palmgren (:mats) 2011-10-30 07:51:37 PDT
Comment on attachment 570504 [details] [diff] [review]
patch

>-  if (NS_SUCCEEDED(aEvent->GetTarget(getter_AddRefs(target))) && target) {
>+  if (mImageFrame &&
>+      NS_SUCCEEDED(aEvent->GetTarget(getter_AddRefs(target))) && target) {

Hmm, this doesn't seem right.  Shouldn't we still do 
area->HasFocus(focus) even if the frame is gone?
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-30 09:21:35 PDT
Well, if mImageFrame is null, ImageMap has been disconnected from imageframe.

I'll move the place of mImageFrame check to be where if (imgFrame) used to be.
Comment 5 alexander :surkov 2011-10-31 07:05:06 PDT
(In reply to Olli Pettay [:smaug] from comment #1)

> surkov, could you test that a11y still works ok.
> It is the only using GetBoundsForAreaContent, IIRC.

Marco, could you make sure imagemaps work and expose correct boundaries please?
Comment 6 Mats Palmgren (:mats) 2011-10-31 17:07:23 PDT
Comment on attachment 570504 [details] [diff] [review]
patch

> I'll move the place of mImageFrame check to be where if (imgFrame) used to
> be.

ok, r=mats with that.
Comment 7 alexander :surkov 2011-11-17 02:06:12 PST
Comment on attachment 570504 [details] [diff] [review]
patch

sorry it took long time, f=me. I don't see a difference between x,y,width,height values between patched trunk Firefox and Firefox 8.
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-27 11:31:11 PST
https://hg.mozilla.org/mozilla-central/rev/b70bd1e1fd1e

Note You need to log in before you can comment on or make changes to this bug.