445968 - Cannot obtain background-color of objects that do not have a color explicitly assigned

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Steps to reproduce:

1. Navigate to the URL mentioned above
2. Use Accerciser to examine the text attributes

Expected results: For the text "This is header 2", the background-color yellow would be exposed. 

Actual results, the background color for that text is reported as transparent. (True with respect to that particular object, but the user wants to know yellow. :-) )

Thanks!

Comment 1

[alexander :surkov (:asurkov)]

Attachment: wip

approach from bug 445509

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (!styleBackground)
+    return CANT_GET_COLOR;

This can never happen. You shouldn't need CANT_GET_COLOR at all.

+  if (parentFrame == aRootFrame)
+    return INHERITED_FROM_ROOT;

I'm not sure what this is for. Can you explain?

Comment 4

[Marco Zehe (:MarcoZ)]

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

r=me for the tests.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> +  if (!styleBackground)
> +    return CANT_GET_COLOR;
> 
> This can never happen. You shouldn't need CANT_GET_COLOR at all.

Ok, I got rid it.

> +  if (parentFrame == aRootFrame)
> +    return INHERITED_FROM_ROOT;
> 
> I'm not sure what this is for. Can you explain?
> 

I put the comment there. This is true if all frames from initial frame to parent frame have transparent background. This helps when I compare background colors of two frames.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd like to understand what you're trying to do by having getColor "inherit" the background. There are situations where the actual background behind the text is not the background of an ancestor.

Also all your method names should start with a capital letter.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> I'd like to understand what you're trying to do by having getColor "inherit"
> the background. There are situations where the actual background behind the
> text is not the background of an ancestor.

That's I missed. How can I get actual background?

> Also all your method names should start with a capital letter.
> 

all methods of all objects of nsTextUtils don't start with a capital letter. I just followed this rule in this patch.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What if there is no actual background color? I mean, what if the text is over an image? It doesn't really make sense to ask for a background color.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> What if there is no actual background color? I mean, what if the text is over
> an image? It doesn't really make sense to ask for a background color.
> 

The meantime we ignore background images so if there is no actual background color then I think I should use default background color. The question is what is the algorithm to find actual background color if navigation through the parents may not give it?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> I'd like to understand what you're trying to do by having getColor "inherit"
> the background. There are situations where the actual background behind the
> text is not the background of an ancestor.

Robert, can you show an example?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

It be obvious. The same effects can be obtained using CSS positioning and other ways.

Comment 13

[Aaron Leventhal]

Let's just remember that the a11y module just needs to get make it possible for authors to communicate their intent to users. The obvious ways of doing background colors should work. We have to cut off at some point depending on cost/benefit.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's quite common for pages to put lots of content in absolutely-positioned blocks. I'm not sure how often you're going to get the "wrong" background color this way.

Can you explain why this information is needed and how it's going to be used? And why does it matter whether the color is "inherited" from the root or not?

Comment 15

[Aaron Leventhal]

It's so that when a blind user is reading or editing documents, they can see when the background color changes in text. For example, a wiki system might mark all "text waiting for approval" in green or something, and the user needs to know which text is green.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, that makes sense.

So for the patch, I don't know why we need INHERITED_FROM_ROOT and this rule stuff. Can't we just walk from the frame up to the root frame to find the first one with a non-transparent background color, and store that, and have equal() check the colors for equality?

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch4

Robert, did you have in view something like this?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That seems better. But why even store mColor/mRootColor? Just store the frames and get the colors as needed.

+ * XXX: Eventually we should get rid this class and replace it by classes
+ * like nsBackgroundTextAttr class declared below. The difference between them
+ * is implementation of nsBackgroundTextAttr class based on nsIFrame methods but
+ * this class implementation is based on nsIDOMCSSStyleDeclaration interface
+ * which is less perfomant (see bug 445509).

If you're going to always be producing strings to pass back to JS, then it's just as efficient, and simpler, to use nsIDOMCSSStyleDeclaration to generate those strings. Of course in this patch you can't do that because of the need to check the ancestor frames.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> That seems better. But why even store mColor/mRootColor? Just store the frames
> and get the colors as needed.

Sure I could store frames but I wouldn't like to get them every time (though probably it's performance issue) and I'm afraid nsIFrame pointer can die eventually. Why do you prefer to store nsIFrame pointers?

> + * XXX: Eventually we should get rid this class and replace it by classes
> + * like nsBackgroundTextAttr class declared below. The difference between them
> + * is implementation of nsBackgroundTextAttr class based on nsIFrame methods
> but
> + * this class implementation is based on nsIDOMCSSStyleDeclaration interface
> + * which is less perfomant (see bug 445509).
> 
> If you're going to always be producing strings to pass back to JS, then it's
> just as efficient, and simpler, to use nsIDOMCSSStyleDeclaration to generate
> those strings. Of course in this patch you can't do that because of the need to
> check the ancestor frames.
> 

In general Js is not primary target for our interfaces. Though of course they can be used and used actually. Here I think we should get some speed win if we won't get strings and compare them to find out if colors are equal.

Comment 20

[Aaron Leventhal]

Comment on attachment 331282 [details] [diff] [review]
patch4

Waiting for response from Roc to surkov's comments. Cancelling review request until discussion is settled.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #19)
> (In reply to comment #18)
> > That seems better. But why even store mColor/mRootColor? Just store the
> > frames and get the colors as needed.
> 
> Sure I could store frames but I wouldn't like to get them every time (though
> probably it's performance issue) and I'm afraid nsIFrame pointer can die
> eventually. Why do you prefer to store nsIFrame pointers?

It's simpler. Performance is not important here. nsBackgroundTextAttr objects are all short-lived right? Frames won't die while you're just setting accessiblity attributes.

> In general Js is not primary target for our interfaces. Though of course they
> can be used and used actually. Here I think we should get some speed win if we
> won't get strings and compare them to find out if colors are equal.

I don't think speed matters here. Let's aim for the simplest code.

Comment 22

[Aaron Leventhal]

I think performance could matter. Screen readers like NVDA, JAWS and Window-Eyes like to walk the entire document on page load to give users a summary, and so that navigation to items is very fast. For example, they may implement a command to navigate to the next formatted text.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's not worry about it until you have profile data that says this code is a bottleneck.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > That seems better. But why even store mColor/mRootColor? Just store the
> > > frames and get the colors as needed.
> > 
> > Sure I could store frames but I wouldn't like to get them every time (though
> > probably it's performance issue) and I'm afraid nsIFrame pointer can die
> > eventually. Why do you prefer to store nsIFrame pointers?
> 
> It's simpler. Performance is not important here. nsBackgroundTextAttr objects
> are all short-lived right? Frames won't die while you're just setting
> accessiblity attributes.

Not sure I get why it's simpler because collor getting is navigation through the tree  (I mean nsBackgroundTextAttr::getColor). Robert, can you clarify your point please?

> > In general Js is not primary target for our interfaces. Though of course they
> > can be used and used actually. Here I think we should get some speed win if we
> > won't get strings and compare them to find out if colors are equal.
> 
> I don't think speed matters here. Let's aim for the simplest code.
> 

Ok. I'll remove that comment and promise I won't fix the bug 445509 until we get confirmation it's a bottleneck.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Calling getColor(mFrame) or getColor(mRootFrame) is simpler than storing those values in fields, because when you cache stuff in fields you have to remember and maintain the invariant that the field has the cached value.

Comment 26

[alexander :surkov (:asurkov)]

Attachment: patch5

store nsIFrame instead of nscolor.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 335826 [details] [diff] [review]
patch5

Thanks.

Could I ask someone to file a bug to get all these function names starting with lowercase letters changed to start with uppercase?

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> (From update of attachment 335826 [details] [diff] [review])
> Thanks.
> 
> Could I ask someone to file a bug to get all these function names starting with
> lowercase letters changed to start with uppercase?
> 

I filed bug 452548 for this.

Comment 29

[alexander :surkov (:asurkov)]

checked in, http://hg.mozilla.org/mozilla-central/index.cgi/rev/c467394773ed