Last Comment Bug 732389 - image map accessible tree is not updated when image map is changed
: image map accessible tree is not updated when image map is changed
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: alexander :surkov
:
Mentors:
Depends on: 738146 736944
Blocks: treeupdatea11y 729831
  Show dependency treegraph
 
Reported: 2012-03-02 05:53 PST by alexander :surkov
Modified: 2012-03-21 22:01 PDT (History)
3 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (38.94 KB, patch)
2012-03-05 06:40 PST, alexander :surkov
bzbarsky: review-
Details | Diff | Splinter Review
patch2 (39.38 KB, patch)
2012-03-13 15:45 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (39.40 KB, patch)
2012-03-13 16:39 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
patch4 (43.05 KB, patch)
2012-03-14 14:44 PDT, alexander :surkov
bzbarsky: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-03-02 05:53:21 PST

    
Comment 1 alexander :surkov 2012-03-05 06:40:33 PST
Created attachment 602882 [details] [diff] [review]
patch
Comment 2 Boris Zbarsky [:bz] 2012-03-08 20:22:45 PST
Comment on attachment 602882 [details] [diff] [review]
patch

>+++ b/accessible/src/html/nsHTMLImageMapAccessible.cpp
>+nsHTMLImageMapAccessible::UpdateChildAreas(bool aDoFireEvents)
>+    if (!area->GetContent()->GetPrimaryFrame()) {

I'm not sure why this makes sense.  What if that content is now just part of some _other_ imagemap?  It can still have a frame, just a different one.

>+  // Insert new areas into the tree.
>+  PRUint32 areaElmCount = imageMapObj->AreaCount();
>+  for (PRUint32 idx = 0; idx < areaElmCount; idx++) {
>+    nsIContent* areaContent = imageMapObj->GetAreaAt(idx);
>+
>+    nsAccessible* area = mChildren.SafeElementAt(idx);

So say an <area> is inserted.  Then all the indices after that point will be off.  We'll insert a bunch of new area accessibles... where do we remove some?

>+++ b/layout/generic/nsImageFrame.cpp
> nsImageFrame::DisconnectMap()
>+#ifdef ACCESSIBILITY
>+  nsAccessibilityService* accService = GetAccService();
>+  if (accService) {
>+    accService->RecreateAccessible(PresContext()->PresShell(), mContent);

Shouldn't this only happen when a11y is active?

>+nsImageFrame::HasImageMap() const

Why not just use GetImageMap()?

>+++ b/layout/generic/nsImageFrame.h
>+  nsImageMap* GetImageMapIfInitialized() const { return mImageMap; }

Get ExistingImageMap, perhaps?

I see no provisions for image maps going away and coming back dynamically in this new code.... am I just missing it?

>+++ b/layout/generic/nsImageMap.cpp
>+#ifdef ACCESSIBILITY
>+  if (NS_SUCCEEDED(rv)) {
>+    nsAccessibilityService* accService = GetAccService();
>+    if (accService) {
>+      accService->UpdateImageMap(mImageFrame);

Shouldn't this be conditioned on a11y being active?
Comment 3 Trevor Saunders (:tbsaunde) 2012-03-08 22:18:07 PST
> >+++ b/layout/generic/nsImageMap.cpp
> >+#ifdef ACCESSIBILITY
> >+  if (NS_SUCCEEDED(rv)) {
> >+    nsAccessibilityService* accService = GetAccService();
> >+    if (accService) {
> >+      accService->UpdateImageMap(mImageFrame);
> 
> Shouldn't this be conditioned on a11y being active?


it is because if a11y is off then there is no accService.
Comment 4 alexander :surkov 2012-03-09 06:39:25 PST
(In reply to Boris Zbarsky (:bz) from comment #2)
> Comment on attachment 602882 [details] [diff] [review]
> patch
> 
> >+++ b/accessible/src/html/nsHTMLImageMapAccessible.cpp
> >+nsHTMLImageMapAccessible::UpdateChildAreas(bool aDoFireEvents)
> >+    if (!area->GetContent()->GetPrimaryFrame()) {
> 
> I'm not sure why this makes sense.  What if that content is now just part of
> some _other_ imagemap?  It can still have a frame, just a different one.

mm, I thought that's impossible to do without notification to old image map and then to new image map. Otherwise we get into the case on layout level when old image map things it owns area while it doesn't. Is it valid scenario?

> >+  // Insert new areas into the tree.
> >+  PRUint32 areaElmCount = imageMapObj->AreaCount();
> >+  for (PRUint32 idx = 0; idx < areaElmCount; idx++) {
> >+    nsIContent* areaContent = imageMapObj->GetAreaAt(idx);
> >+
> >+    nsAccessible* area = mChildren.SafeElementAt(idx);
> 
> So say an <area> is inserted.  Then all the indices after that point will be
> off.  We'll insert a bunch of new area accessibles... where do we remove
> some?

I see, will fix it.

> >+++ b/layout/generic/nsImageFrame.cpp

> >+nsImageFrame::HasImageMap() const
>
> Why not just use GetImageMap()?

I wanted to prevent a11y to trigger image map areas update.
 
> >+++ b/layout/generic/nsImageFrame.h
> >+  nsImageMap* GetImageMapIfInitialized() const { return mImageMap; }
> 
> Get ExistingImageMap, perhaps?

ok

> 
> I see no provisions for image maps going away and coming back dynamically in
> this new code.... am I just missing it?

they fall under nsImageFrame::DisconnectMap(). Did I miss some cases?

> 
> >+++ b/layout/generic/nsImageMap.cpp
> >+#ifdef ACCESSIBILITY
> >+  if (NS_SUCCEEDED(rv)) {
> >+    nsAccessibilityService* accService = GetAccService();
> >+    if (accService) {
> >+      accService->UpdateImageMap(mImageFrame);
> 
> Shouldn't this be conditioned on a11y being active?

as Trevor said it doesn't trigger a11y.
Comment 5 Trevor Saunders (:tbsaunde) 2012-03-09 09:02:44 PST
Comment on attachment 602882 [details] [diff] [review]
patch

>+  nsIPresShell* presShell = aImageFrame->PresContext()->PresShell();
>+  nsDocAccessible* document = GetDocAccessible(presShell->GetDocument());
>+  if (document) {
>+    nsAccessible* accessible =
>+      document->GetAccessible(aImageFrame->GetContent());
>+    if (accessible) {
>+      nsHTMLImageMapAccessible* imageMap = accessible->AsImageMap();
>+      if (imageMap) {
>+        imageMap->UpdateChildAreas();
>+        return;
>+      }
>+
>+      // If image map was initialized after we created an accessible (that'll
>+      // be an image accessible) then recreate it.
>+      RecreateAccessible(aImageFrame->PresContext()->PresShell(),
>+                         aImageFrame->GetContent());

it seems like some local vars would save some recompuation here.

>+    eImageMapAccessible = 1 << 11,

eHTMLImageMapAccessible?

>+nsHTMLImageMapAccessible::UpdateChildAreas(bool aDoFireEvents)

kind of non-normal way to handle update, but ok, do you plan to do more stuff this way in future?

>+  for (PRUint32 idx = 0; idx < areaElmCount; idx++) {
>+    nsIContent* areaContent = imageMapObj->GetAreaAt(idx);
>+
>+    nsAccessible* area = mChildren.SafeElementAt(idx);

so, this only works if image map accessibles first n children are alway the same as the n areas under the image map, which it isn't clear to be needs to be the case.

>+  void UpdateChildAreas(bool aDoFireEvents = true);

do we really need to use optional args here? it seems like just passing true would make things a little clearer.
Comment 6 Boris Zbarsky [:bz] 2012-03-09 09:41:38 PST
> I thought that's impossible to do without notification to old image map and then to new
> image map

That's correct.  Is your code running sync from such notifications?

> I wanted to prevent a11y to trigger image map areas update.

Why?  The update is just somewhat lazy; triggering it earlier is not a big deal imo.

> they fall under nsImageFrame::DisconnectMap(). Did I miss some cases?

Ah, yes.  OK.
Comment 7 alexander :surkov 2012-03-13 06:55:32 PDT
(In reply to Boris Zbarsky (:bz) from comment #2)

> >+  // Insert new areas into the tree.
> >+  PRUint32 areaElmCount = imageMapObj->AreaCount();
> >+  for (PRUint32 idx = 0; idx < areaElmCount; idx++) {
> >+    nsIContent* areaContent = imageMapObj->GetAreaAt(idx);
> >+
> >+    nsAccessible* area = mChildren.SafeElementAt(idx);
> 
> So say an <area> is inserted.  Then all the indices after that point will be
> off.  We'll insert a bunch of new area accessibles... where do we remove
> some?

not sure I follow now. Let's imageMapObj has two areas (area at 0 index was inserted) so mChildren contains child (for second area). For 0 index we insert new area at 0 index into mChildren. for 1 index mChildren[1] equals to second area. Do I miss something?

(In reply to Boris Zbarsky (:bz) from comment #6)
> > I thought that's impossible to do without notification to old image map and then to new
> > image map
> 
> That's correct.  Is your code running sync from such notifications?

yep, sync.

> > I wanted to prevent a11y to trigger image map areas update.
> 
> Why?  The update is just somewhat lazy; triggering it earlier is not a big
> deal imo.

that gives us some reentrances, so let's imagemap frame was asked for accessible, we get image map object which triggers a11y notification that image map was updated and since we don't have an accessible yet then we create an accessible. That doesn't kill us but something not good.
Comment 8 Boris Zbarsky [:bz] 2012-03-13 07:53:09 PDT
> For 0 index we insert new area at 0 index into mChildren. for 1 index mChildren[1]
> equals to second area.

Oh, I see.  Modulo InsertChildAt failures in the loop (how should you deal with those?) this should indeed work, I think....

> that gives us some reentrances

Ah, ok.  Document, please?  And refactor so that HasImageMap() and GetImageMap() don't duplicate all that code.
Comment 9 alexander :surkov 2012-03-13 15:45:26 PDT
Created attachment 605576 [details] [diff] [review]
patch2
Comment 10 alexander :surkov 2012-03-13 16:39:46 PDT
Created attachment 605594 [details] [diff] [review]
patch
Comment 11 Trevor Saunders (:tbsaunde) 2012-03-14 10:41:15 PDT
(In reply to Boris Zbarsky (:bz) from comment #8)
> > For 0 index we insert new area at 0 index into mChildren. for 1 index mChildren[1]
> > equals to second area.
> 
> Oh, I see.  Modulo InsertChildAt failures in the loop (how should you deal
> with those?) this should indeed work, I think....

well, as I understand it when nsTArray uses an infalible alocator inserting elements never fails right?  so InsertChildAt() really should just not be failable in the first place, but I think the approach of just not adding children for new things in the image map should be ok for now.
Comment 12 Trevor Saunders (:tbsaunde) 2012-03-14 11:37:23 PDT
Comment on attachment 605594 [details] [diff] [review]
patch

r=me for accessible/

>+  nsIPresShell* presShell = aImageFrame->PresContext()->PresShell();
>+  nsDocAccessible* document = GetDocAccessible(presShell->GetDocument());
>+  if (document) {
>+    nsAccessible* accessible =
>+      document->GetAccessible(aImageFrame->GetContent());
>+    if (accessible) {
>+      nsHTMLImageMapAccessible* imageMap = accessible->AsImageMap();
>+      if (imageMap) {
>+        imageMap->UpdateChildAreas();
>+        return;
>+      }
>+
>+      // If image map was initialized after we created an accessible (that'll
>+      // be an image accessible) then recreate it.
>+      RecreateAccessible(aImageFrame->PresContext()->PresShell(),

couldn't you use presShell here?

>+  // Remove areas that are not a valid part of the image map anymore.
>+  for (PRInt32 childIdx = mChildren.Length() - 1; childIdx >= 0; childIdx--) {
>+    nsAccessible* area = mChildren.ElementAt(childIdx);
>+    if (!area->GetContent()->GetPrimaryFrame()) {

it might be clearer to continue in the case it does have a frame

>+    function insertArea(aImageMapID, aMapID)
>+        this.mapNode.appendChild(areaElm);

it'd be nice to test other insertion cases than as the last node.

>+      gQueue.push(new insertArea("imgmap", "map"));
>+      gQueue.push(new removeArea("imgmap", "map"));
>+      gQueue.push(new removeNameOnMap("container", "imgmap", "map"));
>+      gQueue.push(new restoreNameOnMap("container", "imgmap", "map"));
>+      gQueue.push(new removeMap("container", "imgmap", "map"));

test inserting a map too?
Comment 13 Trevor Saunders (:tbsaunde) 2012-03-14 11:39:35 PDT
Comment on attachment 605594 [details] [diff] [review]
patch

fixing outstanding  review request
Comment 14 alexander :surkov 2012-03-14 14:44:24 PDT
Created attachment 605961 [details] [diff] [review]
patch4

trevor's comments addressed
Comment 15 Boris Zbarsky [:bz] 2012-03-15 10:03:21 PDT
Comment on attachment 605961 [details] [diff] [review]
patch4

>+++ b/layout/generic/nsImageFrame.h
>+  mozilla::dom::Element* GetMapElm() const

GetMapElement, please.

r=me with that.
Comment 16 alexander :surkov 2012-03-15 13:19:12 PDT
landed with Boris comment addressed https://hg.mozilla.org/integration/mozilla-inbound/rev/d81fe7ffabe1
Comment 17 Marco Bonardo [::mak] 2012-03-16 06:19:39 PDT
https://hg.mozilla.org/mozilla-central/rev/d81fe7ffabe1

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