Closed
Bug 634197
Opened 13 years ago
Closed 13 years ago
assert if accessible tree update triggers reflow
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file, 1 obsolete file)
4.67 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
spun off the bug 629912 comment #36
Assignee | ||
Comment 1•13 years ago
|
||
Boris, do I get right one of such bad things is bug 630486?
Comment 2•13 years ago
|
||
OK to paste a bit of that: We have to be careful: 1) Any time you flush. 2) Any time you call code you don't control and that might flush. Examples of #2 would be any DOM API asking for position information, say. Or scrolling. Or anything that spins the event loop, of course." (I don't have access to bug 630486) I agree this should block until we figure things out here.
blocking2.0: ? → final+
Whiteboard: [hardblocker]
Assignee | ||
Comment 3•13 years ago
|
||
Boris, is it ok to call nsIFrame::GetRenderedText (we do that when refresh driver calls into us and we update the tree)?
Comment 4•13 years ago
|
||
> Boris, do I get right one of such bad things is bug 630486?
That one doesn't look bad, as long as the a11y code on the stack can deal with arbitrary script running.
The refresh driver expects that arbitrary script might run under the WillRefresh call it makes, so that part is fine.
Comment 5•13 years ago
|
||
> Boris, is it ok to call nsIFrame::GetRenderedText
Yes.
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to comment #4) > > Boris, do I get right one of such bad things is bug 630486? > > That one doesn't look bad, as long as the a11y code on the stack can deal with > arbitrary script running. > > The refresh driver expects that arbitrary script might run under the > WillRefresh call it makes, so that part is fine. It doesn't trigger text reflow? It always happen after timeout? (when we create accessible tree we and run popup children creation).
Assignee | ||
Comment 8•13 years ago
|
||
(In reply to comment #7) > I'm not sure what comment 6 is asking. I'm trying figure out if "Any time you call code you don't control and that might flush." possible. Is the following scenario valid? 1) refresh driver calls into us 2) we create an accessible tree 3) trigger popup frame children creation 4) getting text reflow
Comment 9•13 years ago
|
||
That's fine from the refresh driver's pov if you're doing it under WillRefresh. So as long as _your_ code is ok with that, it's fine.
Updated•13 years ago
|
Whiteboard: [hardblocker] → [hardblocker] see possible dupe bug 629912
Comment 10•13 years ago
|
||
I wonder if there is a way instrument against causing a flush at the wrong time using static analysis and source annotation?
Comment 11•13 years ago
|
||
Alexander I'm moving this to blocking .x pending further investigation. Please renominate with rationale if you disagree.
blocking2.0: final+ → .x
Whiteboard: [hardblocker] see possible dupe bug 629912
Comment 12•13 years ago
|
||
Note I felt we didn't need to hard block anymore given Bz's recent comment: bug 634200 comment 2
Assignee | ||
Comment 13•13 years ago
|
||
We don't have other explicit layout flush in a11y. We could close this bug as fixed but it'd be nice to assert if something flush (or reflows) while we're inside WillRefresh. Boris, is there a way to add an assertion for that?
blocking2.0: .x → ---
Comment 14•13 years ago
|
||
You want to assert that you're not entering PresShell::FlushPendingNotifications while you're inside your own WillRefresh callback?
Assignee | ||
Comment 15•13 years ago
|
||
(In reply to comment #14) > You want to assert that you're not entering > PresShell::FlushPendingNotifications while you're inside your own WillRefresh > callback? That'd be great. I don't know if reflow is different from FlushPendingNotifications, i.e. can be triggered by something else.
Comment 16•13 years ago
|
||
It can, but only if documents are resized. I suppose you could just add some state on the accessibility service or something and assert it in presshell....
Assignee | ||
Updated•13 years ago
|
Blocks: treeupdatea11y
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #564482 -
Flags: review?(bzbarsky)
Attachment #564482 -
Flags: review?(bolterbugz)
Comment 18•13 years ago
|
||
Comment on attachment 564482 [details] [diff] [review] patch This is just debugging code, right?
Assignee | ||
Comment 19•13 years ago
|
||
yes
Comment 20•13 years ago
|
||
Comment on attachment 564482 [details] [diff] [review] patch Do the asserts hit?
Attachment #564482 -
Flags: review?(bolterbugz) → review+
Comment 21•13 years ago
|
||
Comment on attachment 564482 [details] [diff] [review] patch IsLayoutRefreshProcessed is a really odd name for this function, since it implies that something has _already_ happened. What does it actually mean for this function to return true? It seems to mean that something _is_ happening, right?
Assignee | ||
Comment 22•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #21) > Comment on attachment 564482 [details] [diff] [review] [diff] [details] [review] > patch > > IsLayoutRefreshProcessed is a really odd name for this function, since it > implies that something has _already_ happened. What does it actually mean > for this function to return true? It seems to mean that something _is_ > happening, right? right, thank you, oversight. IsLayoutRefreshProcessing is better?
Comment 23•13 years ago
|
||
Is it a layout refresh, or a refresh of some a11y data structures that's happening?
Assignee | ||
Comment 24•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #23) > Is it a layout refresh, or a refresh of some a11y data structures that's > happening? refresh of a11y
Comment 25•13 years ago
|
||
OK, then LayoutRefresh doesn't sound right.... Maybe just IsProcessingRefreshDriverNotification() if that's what it really means?
Assignee | ||
Comment 26•13 years ago
|
||
sounds good, thank you. a little bit long though
Assignee | ||
Comment 27•13 years ago
|
||
Attachment #564482 -
Attachment is obsolete: true
Attachment #564839 -
Flags: review?(bzbarsky)
Attachment #564482 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Summary: make sure accessible tree creation doesn't trigger reflow → assert if accessible tree update triggers reflow
Comment 28•13 years ago
|
||
Comment on attachment 564839 [details] [diff] [review] patch2 r=me
Attachment #564839 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 29•13 years ago
|
||
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/00dcb64cb920
Comment 30•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/00dcb64cb920
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in
before you can comment on or make changes to this bug.
Description
•