732389 - image map accessible tree is not updated when image map is changed

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

> >+++ 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 (:asurkov)]

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

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 [:bzbarsky]]

> 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 (:asurkov)]

(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 [:bzbarsky]]

> 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 (:asurkov)]

Attachment: patch2

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 11

[Trevor Saunders (:tbsaunde)]

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

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

Comment on attachment 605594 [details] [diff] [review]
patch

fixing outstanding  review request

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch4

trevor's comments addressed

Comment 15

[Boris Zbarsky [:bzbarsky]]

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 (:asurkov)]

landed with Boris comment addressed https://hg.mozilla.org/integration/mozilla-inbound/rev/d81fe7ffabe1

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/d81fe7ffabe1