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

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {access})

unspecified
mozilla14
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 602882 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #602882 - Flags: review?(trev.saunders)
Attachment #602882 - Flags: review?(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?
Attachment #602882 - Flags: review?(bzbarsky) → review-
> >+++ 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.
(Assignee)

Comment 4

5 years ago
(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 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.
Attachment #602882 - Flags: review?(trev.saunders)
> 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.
(Assignee)

Comment 7

5 years ago
(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.
> 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.
(Assignee)

Comment 9

5 years ago
Created attachment 605576 [details] [diff] [review]
patch2
Attachment #602882 - Attachment is obsolete: true
Attachment #605576 - Flags: review?(trev.saunders)
Attachment #605576 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #605576 - Attachment is obsolete: true
Attachment #605576 - Flags: review?(trev.saunders)
Attachment #605576 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

5 years ago
Created attachment 605594 [details] [diff] [review]
patch
Attachment #605594 - Flags: review?(trev.saunders)
Attachment #605594 - Flags: review?(bzbarsky)
(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 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?
Attachment #605594 - Flags: review?(bzbarsky) → review+
Comment on attachment 605594 [details] [diff] [review]
patch

fixing outstanding  review request
Attachment #605594 - Flags: review?(trev.saunders) → review?(bzbarsky)
(Assignee)

Comment 14

5 years ago
Created attachment 605961 [details] [diff] [review]
patch4

trevor's comments addressed
Attachment #605594 - Attachment is obsolete: true
Attachment #605961 - Flags: review?(bzbarsky)
Attachment #605594 - Flags: review?(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.
Attachment #605961 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 16

5 years ago
landed with Boris comment addressed https://hg.mozilla.org/integration/mozilla-inbound/rev/d81fe7ffabe1
https://hg.mozilla.org/mozilla-central/rev/d81fe7ffabe1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14

Updated

5 years ago
Depends on: 736944
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Depends on: 738146
You need to log in before you can comment on or make changes to this bug.