90624 - Active Accessibility: accGetBounds returning incorrect values

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Aaron Leventhal]

Our implementation of accGetBounds is incorrect.
It returns incorrect values for links that contains images, and for any text
that flows to a new line. It's also difficult to follow the old code.

A new implementation is needed.

Comment 1

[Aaron Leventhal]

Attachment: Replaces a whole lot of non-working code with something small that works right

Comment 2

[Aaron Leventhal]

John and Chris, I test this and it works.
Ready for review/superreview

Comment 3

[John Gaunt (redfive)]

- you assign aRelativeParent to the parent of the BoundsFrame and then before 
using it reassign it:

  firstFrame->GetParent(aRelativeParent);

  then:

  *aRelativeParent = parentFrame;

  It is in a while condition, but if we don't enter the while basically the rest 
of the function doesn't get executed either because we have no frames around. ( 
trace back the assignment into parentFrame -- first time through, no 
parentFrame, no BoundsFrame. so we always enter the while at least once )

- You reuse the variable parentFrame, is there a different name you can use?
  // Find common relative parent
  nsIFrame *parentFrame = iterFrame;

  while (iterContent == firstContent || depth > 0) {
    // Coordinates will come back relative to parent frame
    nsIFrame *parentFrame = iterFrame;

  if nothing else, maybe not creating a new parentFrame.

Comment 4

[Aaron Leventhal]

Attachment: patch #2

Comment 5

[John Gaunt (redfive)]

r=jgaunt on patch 42226

Comment 6

[John Gaunt (redfive)]

Removing waterson ( on vacation ) 
adding brendan for sr=

Comment 7

[Brendan Eich [:brendan]]

Cool -- patches should try to remove code rather than add code, to fix buts --
or else code tends to bloat over time!

+  while (ancestorFrame) {  
+    *aRelativeParent = ancestorFrame;
+    if (IsCorrectFrameType(ancestorFrame, nsLayoutAtoms::blockFrame) || 
+      IsCorrectFrameType(ancestorFrame, nsLayoutAtoms::areaFrame))
+      break;
+    ancestorFrame->GetParent(&ancestorFrame); 
+  }

Shouldn't you set *aRelativeParent = nsnull; before the loop, in case it does
not iterate?

Style nit: Wouldn't it look better if the underhanging part of the condition
were indented so that IsCorrectFrameType lined up (started in the same column on
both lines)? You might even brace the break; so the statement boundaries are
clearer at a glance.

Style nit: Wouldn't aRelativeAncestor be a better name?  Actually, the
"relative" part is confusion when mixed with "ancestor" or "parent" -- why not
aBoundingFrame or some such?

I studied enough of the big hair frame-tree walking loop to believe that it
works as designed.  One nit only:

+        if (!iterNextFrame && --depth >= 0) 
+          iterFrame->GetParent(&iterFrame);
+        else break;

Maybe put the break on its own line, so you can breakpoint it cleanly on all
debuggers?  That also suggests applying De Morgan's theorem:

+        if (iterNextFrame || --depth < 0) 
+          break;
+        iterFrame->GetParent(&iterFrame);

This also shows more clearly (to me, at any rate) that depth can and should go
negative (to -1), which helps motivate its signed-ness.

I am so not a frames jock that I'm cc'ing waterson (in case he cares when he
gets back), attinasi, and dbaron.  Marc, David: if one or both of you could take
a look and give a benediction, I'll owe you.  Not an obstacle to checkin, but a
sanity check that might result in sequel bugs or last-minute saves.

Pick my nits, heed attinasi&dbaron if they come back with comments fast, and
sr=[e-mail].  If you really need to get this into the trunk, I don't
see it doing any harm, so I'm giving a real sr= here -- but I hope to get backup
review from a frames guru, because I'm not one.

/be

Comment 8

[Aaron Leventhal]

Attachment: New patch with Brendan's suggestions

Comment 9

[Brendan Eich [:brendan]]

Set *aBoundingFrame = nsnull; before any return, was what I had in mind when I
wrote "Shouldn't you set *aRelativeParent = nsnull; before the loop, in case it
does not iterate?"  If aBoundingFrame is a pure out parameter pointer (to a
frame pointer), the caller should not have to initialize the frame pointer that
it passes to nsnull, in order to have a well-defined return value in all cases.

/be

Comment 10

[Aaron Leventhal]

I have added Brendan's last suggestion of setting the out parameter to null
right .away

It's implemented  in my local tree.

Comment 11

[Marc Attinasi]

The patch looks good to me, but I am curious if the check

  if (IsCorrectFrameType(iterFrame, nsLayoutAtoms::inlineFrame)) {

is sufficient. I mean, are there inline frames that will return some other type,
I am not sure. It might be worth checking, though this probably covers most
cases. r=attinasi

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think that nsLayoutAtoms::inlineFrame check should be sufficient since the 
only ones he cares about are those that can have descendents outside of their 
bounds, which are only non-replaced inline elements, and should thus all be 
standard inline frames.

Comment 13

[Aaron Leventhal]

Chofmann has approved via email.

Comment 14

[Aaron Leventhal]

Fix checked into branch as well.