Closed
Bug 731287
Opened 9 years ago
Closed 9 years ago
don't use GetComputedStyle for background-color in HTML table layout guess
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: surkov, Assigned: surkov)
References
Details
Attachments
(2 files)
1.81 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
7.19 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #601316 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 1•9 years ago
|
||
and remove computed styles from nsCoreUtils to keep out of mischief
Attachment #601321 -
Flags: review?(trev.saunders)
Comment 2•9 years ago
|
||
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.
Attachment #601321 -
Flags: review?(trev.saunders) → review+
Comment 3•9 years ago
|
||
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.
Attachment #601316 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 4•9 years ago
|
||
(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
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to alexander :surkov from comment #0) > Created attachment 601316 [details] [diff] [review] > patch https://hg.mozilla.org/integration/mozilla-inbound/rev/110d4d884186
Whiteboard: [don't mark fixed]
Assignee | ||
Comment 6•9 years ago
|
||
(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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/110d4d884186
Target Milestone: --- → mozilla13
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
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)
Keywords: dev-doc-needed
Assignee | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdab1eea1212
Keywords: dev-doc-needed
Whiteboard: [don't mark fixed]
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bdab1eea1212
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 13•9 years ago
|
||
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
Assignee | ||
Comment 14•9 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•