Created attachment 601316 [details] [diff] [review] patch
Created attachment 601321 [details] [diff] [review] remove from nsCoreUtils and remove computed styles from nsCoreUtils to keep out of mischief
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 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.
(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
(In reply to alexander :surkov from comment #0) > Created attachment 601316 [details] [diff] [review] > patch https://hg.mozilla.org/integration/mozilla-inbound/rev/110d4d884186
(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.
https://hg.mozilla.org/mozilla-central/rev/110d4d884186
(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.
(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.
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)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdab1eea1212
https://hg.mozilla.org/mozilla-central/rev/bdab1eea1212
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
(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.
or
