731287 - don't use GetComputedStyle for background-color in HTML table layout guess

Status: RESOLVED FIXED

Description

[alexander :surkov (:asurkov)]

Created attachment 601316 [details] [diff] [review]
patch

Comment 1

[alexander :surkov (:asurkov)]

Created attachment 601321 [details] [diff] [review]
remove from nsCoreUtils

and remove computed styles from nsCoreUtils to keep out of mischief

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 601321 [details] [diff] [review]
remove from nsCoreUtils

>@@ -156,17 +157,17 @@ CAccessibleComponent::GetARGBValueFromCS
> __try {
>   *aColorValue = 0;
> 
>   nsRefPtr<nsAccessible> acc(do_QueryObject(this));
>   if (!acc || acc->IsDefunct())
>     return E_FAIL;
> 
>   nsCOMPtr<nsIDOMCSSStyleDeclaration> styleDecl =
>-    nsCoreUtils::GetComputedStyleDeclaration(EmptyString(), acc->GetContent());
>+    nsWinUtils::GetComputedStyleDeclaration(acc->GetContent());

I gues that's fine as we use this function now, but wouldn't this whole function be nicer if we used frame stuff instead?

>   /**
>+   * Return computed styles declaration for the given node.
>+   */
>+  static already_AddRefed<nsIDOMCSSStyleDeclaration>
>+    GetComputedStyleDeclaration(nsIContent* aContent);

so, now we can only shoot ourselves on windows, that seems like an improvement, but it'd be nice to have a big sign saying danger! here.

Comment 3

[Trevor Saunders (:tbsaunde)]

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

>   for (PRUint32 childIdx = 0; childIdx < childCount; childIdx++) {
>     nsAccessible* child = GetChildAt(childIdx);
>     if (child->Role() == roles::ROW) {

couldn't we use AccIterator with filters::row here though I'm not sure I actually like that better.

>+      prevRowColor = rowColor;

comparing to the color of the first row would work equally well, but it makes this a little more complicated I think, and afaik nsColor is just an int so I doubt it matters at all.

>+      nsIFrame* rowFrame = child->GetFrame();
>+      rowColor = rowFrame->GetStyleBackground()->mBackgroundColor;
>+
>+      if (childIdx > 0 && prevRowColor != rowColor)

that's... somewhere between cute and  hax.

>+        RETURN_LAYOUT_ANSWER(false, "2 styles of row background color, non-bordered");

I'm not sure I understand the logic behind this particular huristic, but whatever.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> >   for (PRUint32 childIdx = 0; childIdx < childCount; childIdx++) {
> >     nsAccessible* child = GetChildAt(childIdx);
> >     if (child->Role() == roles::ROW) {
> 
> couldn't we use AccIterator with filters::row here though I'm not sure I
> actually like that better.

we could, it's nice syntax and it's used for similar things in nsARIAGridAccessible but yep I'm not sure I like that better too.

> >+      prevRowColor = rowColor;
> 
> comparing to the color of the first row would work equally well, but it
> makes this a little more complicated I think, and afaik nsColor is just an
> int so I doubt it matters at all.

yes

Comment 5

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #0)
> Created attachment 601316 [details] [diff] [review]
> patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/110d4d884186

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 601321 [details] [diff] [review]
> remove from nsCoreUtils
> 
> >@@ -156,17 +157,17 @@ CAccessibleComponent::GetARGBValueFromCS
> >   nsCOMPtr<nsIDOMCSSStyleDeclaration> styleDecl =
> >-    nsCoreUtils::GetComputedStyleDeclaration(EmptyString(), acc->GetContent());
> >+    nsWinUtils::GetComputedStyleDeclaration(acc->GetContent());
> 
> I gues that's fine as we use this function now, but wouldn't this whole
> function be nicer if we used frame stuff instead?

I'm not sure what you mean by nicer but I would use frame stuffs if that would be easy. They can request any style so at least right now we can't handle that by StyleInfo.

> >+  static already_AddRefed<nsIDOMCSSStyleDeclaration>
> >+    GetComputedStyleDeclaration(nsIContent* aContent);
> 
> so, now we can only shoot ourselves on windows, that seems like an
> improvement, but it'd be nice to have a big sign saying danger! here.

I'd say shutdown ourselves :) AT might be surprised about this however. Or shoot if somebody who doesn't know about this bug will use it somewhere where he shouldn't. I'll add a comment saying it should be used careful.

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/110d4d884186

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > Comment on attachment 601321 [details] [diff] [review]
> > remove from nsCoreUtils
> > 
> > >@@ -156,17 +157,17 @@ CAccessibleComponent::GetARGBValueFromCS
> > >   nsCOMPtr<nsIDOMCSSStyleDeclaration> styleDecl =
> > >-    nsCoreUtils::GetComputedStyleDeclaration(EmptyString(), acc->GetContent());
> > >+    nsWinUtils::GetComputedStyleDeclaration(acc->GetContent());
> > 
> > I gues that's fine as we use this function now, but wouldn't this whole
> > function be nicer if we used frame stuff instead?
> 
> I'm not sure what you mean by nicer but I would use frame stuffs if that
> would be easy. They can request any style so at least right now we can't
> handle that by StyleInfo.

how? this function is a private helper that we only call with constants for the first argument.

> 
> > >+  static already_AddRefed<nsIDOMCSSStyleDeclaration>
> > >+    GetComputedStyleDeclaration(nsIContent* aContent);
> > 
> > so, now we can only shoot ourselves on windows, that seems like an
> > improvement, but it'd be nice to have a big sign saying danger! here.
> 
> I'd say shutdown ourselves :) AT might be surprised about this however. Or
> shoot if somebody who doesn't know about this bug will use it somewhere
> where he shouldn't. I'll add a comment saying it should be used careful.

sure, shutting yourself down and then doing further stuffs with yourself though will almost certainly end poorly.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > >@@ -156,17 +157,17 @@ CAccessibleComponent::GetARGBValueFromCS
> > > >   nsCOMPtr<nsIDOMCSSStyleDeclaration> styleDecl =
> > > >-    nsCoreUtils::GetComputedStyleDeclaration(EmptyString(), acc->GetContent());
> > > >+    nsWinUtils::GetComputedStyleDeclaration(acc->GetContent());
> > > 
> > > I gues that's fine as we use this function now, but wouldn't this whole
> > > function be nicer if we used frame stuff instead?
> > 
> > I'm not sure what you mean by nicer but I would use frame stuffs if that
> > would be easy. They can request any style so at least right now we can't
> > handle that by StyleInfo.
> 
> how? this function is a private helper that we only call with constants for
> the first argument.

I'm not sure whether we talk about the same but if yes then take a look at CAccessibleComponent::GetARGBValueFromCSSProperty(const nsAString& aPropName,
where aPropName is a requested style.

Comment 10

[alexander :surkov (:asurkov)]

actually https://developer.mozilla.org/en/nsIAccessNode and https://developer.mozilla.org/en/nsIAccessible should be updated (nsIAccessNode was removed, all methods were moved to nsIAccessible)

Comment 11

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdab1eea1212

Comment 12

[Marco Bonardo [::mak]]

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

Comment 13

[Ronny Perinke]

I got this warning message on nshtmltableaccessible.cpp(1466) with VS2010 and Auror (15a2)

warning C4700: uninitialized local variable 'rowColor' used.

https://hg.mozilla.org/releases/mozilla-aurora/annotate/43d14e9a5237/accessible/src/html/nsHTMLTableAccessible.cpp#l1466

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Ronny Perinke from comment #13)
> I got this warning message on nshtmltableaccessible.cpp(1466) with VS2010
> and Auror (15a2)
> 
> warning C4700: uninitialized local variable 'rowColor' used.
> 
> https://hg.mozilla.org/releases/mozilla-aurora/annotate/43d14e9a5237/
> accessible/src/html/nsHTMLTableAccessible.cpp#l1466

It was fixed by http://hg.mozilla.org/mozilla-central/diff/1deeb80069cf/accessible/src/html/HTMLTableAccessible.cpp. Thanks for reporting it.