Last Comment Bug 731287 - don't use GetComputedStyle for background-color in HTML table layout guess
: don't use GetComputedStyle for background-color in HTML table layout guess
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: alexander :surkov
:
:
Mentors:
Depends on:
Blocks: 728127
  Show dependency treegraph
 
Reported: 2012-02-28 10:10 PST by alexander :surkov
Modified: 2012-09-04 18:09 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.81 KB, patch)
2012-02-28 10:10 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
remove from nsCoreUtils (7.19 KB, patch)
2012-02-28 10:32 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-02-28 10:10:50 PST
Created attachment 601316 [details] [diff] [review]
patch
Comment 1 alexander :surkov 2012-02-28 10:32:50 PST
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) 2012-03-02 22:03:30 PST
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) 2012-03-02 22:18:19 PST
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 2012-03-03 07:57:12 PST
(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 2012-03-03 18:30:47 PST
(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 2012-03-04 07:48:50 PST
(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] 2012-03-04 15:27:45 PST
https://hg.mozilla.org/mozilla-central/rev/110d4d884186
Comment 8 Trevor Saunders (:tbsaunde) 2012-03-09 09:32:40 PST
(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 2012-03-12 14:02:00 PDT
(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 2012-03-12 14:39:01 PDT
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 12 Marco Bonardo [::mak] 2012-03-13 05:53:26 PDT
https://hg.mozilla.org/mozilla-central/rev/bdab1eea1212
Comment 13 Ronny Perinke 2012-06-11 11:48:07 PDT
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 2012-09-04 18:09:40 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.