We resolve pending animations when painting has finished in
nsDisplayList::PaintRoot. However, in the process we can trigger nested calls to
PaintFrame (e.g. due to use of -moz-element). In that case, we shouldn't resolve
pending animations until we complete the widget transaction.
This patch adds a check that we're in a widget transaction before we resolve
pending animations.
After starting layer animations we set the same start time on content
animations but we don't apply it until the next tick (see bug 1112480 for
background). However, in some circumstances, we can end up creating layer
animations again within the same refresh driver tick. In this case, we should
initialize the animations with the same start time as we previously used.
This patch exposes the pending start time set on content animations so that,
if set, we can use it when building layer animations.
It feels safer to use a function with a new name, rather than just changing the
behaviour of the existing function.
For most of these cases the PL_DHashTableLookup() result was checked with
PL_DHASH_ENTRY_IS_{FREE,BUSY} so the conversion was easy. A few of them
preceded that check with a useless null check, but the intent of these was
still easy to determine.
I'll do the trickier ones in subsequent patches.
--HG--
extra : rebase_source : ab37a7a30be563861ded8631771181aacf054fd4
We do not want to traverse inside native anonymous elements, but we
should still be able to skip over generated content, to avoid getting
stuck on such images.
The caret movement code already handles unselectable text frames if we
happen to land in the middle of one in nsTextFrame::PeekOffsetCharacter/Word.
However, when performing frame traversal to find the next frame to jump
to, we don't remember if we skipped over an unselectable frame, which causes
us to jump one offset too much when the caret is on the boundary of
selectable and unselectable content. The test cases demonstrate the
scenario. Note that an <img alt=foo> is implemented by adding a
generated content to the inline frame representing it, so as far as
the caret movement code is concerned, both test cases are treated similarly.
Note that we need to do this only when moving the selection, and not
when extending it. We are adding an aExtend argument to
nsPeekOffsetStruct's constructor in order to be able to special case
that.
We do not want to traverse inside native anonymous elements, but we
should still be able to skip over generated content, to avoid getting
stuck on such images.
The caret movement code already handles unselectable text frames if we
happen to land in the middle of one in nsTextFrame::PeekOffsetCharacter/Word.
However, when performing frame traversal to find the next frame to jump
to, we don't remember if we skipped over an unselectable frame, which causes
us to jump one offset too much when the caret is on the boundary of
selectable and unselectable content. The test cases demonstrate the
scenario. Note that an <img alt=foo> is implemented by adding a
generated content to the inline frame representing it, so as far as
the caret movement code is concerned, both test cases are treated similarly.
Note that we need to do this only when moving the selection, and not
when extending it. We are adding an aExtend argument to
nsPeekOffsetStruct's constructor in order to be able to special case
that.
Sometimes, in very specific cases, the visible region gets simplified to one rect and is thus much bigger than the draw region. This becomes a problem if we decide to pull an opaque background color from a lower layer so that we are opaque. In which case we draw the background color over the whole visible region. But we use the draw region to determine if we can place items below this layer, so that background color could cover them incorrectly.
The content inside an editable region is either editable itself, or
is inside a contenteditable="false" subtree. In the first case,
it should not be focusable since it is editable. In the second
case, it should not be focusable since the entire non-editable
region is treated as a special single entity for the purposes of
selection and caret movement, and having something focusable in
the middle of such a subtree breaks that model.
The content inside an editable region is either editable itself, or
is inside a contenteditable="false" subtree. In the first case,
it should not be focusable since it is editable. In the second
case, it should not be focusable since the entire non-editable
region is treated as a special single entity for the purposes of
selection and caret movement, and having something focusable in
the middle of such a subtree breaks that model.
Currently the setting of PLDHashTable::ops is very haphazard.
- PLDHashTable has no constructor, so it's not auto-nulled, so lots of places
null it themselves.
- In the fallible PLDHashTable::Init() function, if the entry storage
allocation fails we'll be left with a table that has |ops| set -- indicating
it's been initialized -- but has null entry storage. I'm not certain this can
cause problems but it feels unsafe, and some (but not all) callers of Init()
null it on failure.
- PLDHashTable does not null |ops| in Finish(), so some (but not all) callers
do this themselves.
This patch makes things simpler.
- It adds a constructor that zeroes |ops|.
- It modifies Init() so that it only sets |ops| once success is ensured.
- It zeroes |ops| in Finish().
- Finally, it removes all the now-unnecessary |ops| nulling done by the users
of PLDHashTable.
--HG--
extra : rebase_source : bb34979c218d152562a2f9c7e5215256c111cc5b
I noticed the GetRootPresContext call being expensive in a profile that
involved painting in a (non-e10s) window with around 400-500 tabs.
Moving the mIsActive test (most likely to be false) first should fix
that.
I don't have any tests that exercise this code, and I can't even find a
codepath that demonstrates that it's needed, since the lazy
reconstruction that happens during style-triggered frame reconstruction
all appears to go through PostRestyleEvent rather than
MaybeConstructLazily.
But I think we should either do this or add an assertion that it's not
needed, and given that it's one line, it seems like we may as well just
do it. (Note also that we're currently calling CreateNeededFrames at
the start of style reresolution, in
RestyleManager::ProcessPendingRestyles; this adds a call at the end.)
This makes the ReframingStyleContexts live across the lifetime of the
processing of a full queue of posted restyles.
This depends on bug 1115812 to behave sensibly (and not assert) when
rebuilding the rule tree (RebuildAllStyleData, etc.).
This handles the form of lazy frame construction that is done in
nsCSSFrameConstructor::RecreateFramesForContent, which posts a restyle.
Patch 7 handles any use of the lazy frame construction mechanism.
This patch (with patches 4 and 5 under it, but without patches 1-3)
fixes the original testcase in bug 1110277, except for some flashing of
the final position as the transition starts.
Also fixes bug 1111451.
These assertions are as requested in comment 30 in the bug.
(If the first one doesn't fire, it's possible we could even get rid of
the handling of mDoRebuildAllStyleData that it's within.)
If we discover that we've set mDoRebuildAllStyleData in the middle of
ProcessPendingRestyles(), now that ProcessPendingRestyles() fully
handles mDoRebuildAllStyleData, we only need to make a recursive call to
ProcessPendingRestyles, rather than calling RebuildAllStyleData to call
ProcessPendingRestyles.
This fixes another pre-existing bug in the rebuild-all codepath; it
didn't handle the animation-only update correctly, which could have
caused bugs in transitions with OMT animations enabled.
This means that instead of recurring into DoRebuildAllStyleData, we'll
call StartRebuildAllStyleData in the middle of processing the restyle
queue (which is fine). StartRebuildAllStyleData will move the old rule
tree out of the way and immediately do a full-tree restyle, before
returning to any queue processing that might be left (the full-tree
restyle should have consumed all remaining restyle hints, but might have
posted some new ones for handling reframes that require reframing
ancestors). And, more importantly, the EndReconstruct() call to get rid
of the old rule tree won't happen until after we're done processing the
containing RestyleTracker's queue of restyles, which reduces the risk of
having dangling old style contexts and makes it easier (in bug 1110277)
to have a ReframingStyleContexts with the right lifetime.
This changes what was probably a silly design choice when I wrote the
code for 'rem'-basis handling; we shouldn't try continuing through the
rest of RestyleElement() here, but instead repost the hint to the
rebuild-all process.
This switches RebuildAllStyleData() to the normal
ProcessPendingRestyles() manner of restyle processing. This means a
rebuild-all going through this codepath (the main rebuild-all codepath)
only sets up for non-animation restyle processing once rather than doing
it twice (and potentially having reframes posted in
DoRebuildAllStyleData() that don't get processed until
ProcessPendingRestyles(), which causes a variant of bug 1110277 with
transitions on reframed elements failing to start because it doesn't
match the lifetime of the ReframingStyleContexts).
In the new way of doing a rebuild-all, StartRebuildAllStyleData might be
called directly from ProcessPendingRestyles rather than from
RebuildAllStyleData (which null-checks the root frame) or from within
processing restyles (which can only happen when there's a root frame).
This means it needs its own null-check of the root frame.
Here we call StartRebuildAllStyleData from BeginProcessingRestyles (much
like patch 9 and EndProcessingRestyles). But we will later also call it
from the code that handles a root element font size change when we have
'rem' units. That's because it's fine to *start* the rebuild process in
the middle of processing the queue of pending restyles. (We have to end
after the whole process is done, though, in order to avoid wanting to
destroy the old rule tree while we still have style contexts referencing
it.)
We only call StartRebuildAllStyleData in this case when we're processing
our primary restyle queue (mPendingRestyles), not the animation restyles
(to be removed in bug 960465) or the animation-only restyles, since a
rebuild-all should be processed (in terms of animation phases, or in
terms of having an animation-only update before it) like a normal
restyle. (This isn't true for the 'rem' unit restyle, which could
happen during any sort of update.)
This moves the code that finishes the rebuild-all process into
EndProcessingRestyles(), which is part of the main restyling codepath.
Patch 7 ensures that we'll always get to EndProcessingRestyles in this
case, when we're going through the normal ProcessPendingRestyles()
codepath rather than the special DoRebuildAllStyleData() codepath (which
will be removed later in this patch series).
This fixes one of the omissions in the rebuild-all codepaths (where it
incorrectly differs from the regular ProcessPendingRestyles codepath).
Note that the explicit FlushOverflowChangedTracker() is no longer needed
because that's part of EndProcessingRestyles.
(This will all get refactored more substantially in the following
patches.)
This is needed for patch 9 (once patch 9 is used via the
ProcessPendingRestyles() codepath in patch 13); it ensures that when we
use the new way of rebuilding, we don't bail out early because we think
we have nothing to do.
This adds a member variable that is currently only used within a single
function, but that function will be split apart so that different parts
of it can be called from different places within ProcessPendingRestyles.
This is the variable that says we *need to* rebuild style data. Since
the next patch will introduce a variable that says we're *currently*
rebuilding all style data, renaming this one makes things clearer.
Part of this refactoring involves the ability to start the rebuild-all
process within the processing of restyles. This means we can't pass
parameters directly from RebuildAllStyleData into DoRebuildAllStyleData.
So this continues storing the hints as member variables a little bit
deeper into the process.
(I tried to move in a different direction in this patch queue, and store
these hints in mPendingRestyles, for the root element. But that broke
layout/style/test/test_counter_style.html and
layout/style/test/test_font_loading_api.html, and I didn't want to
figure out why. It would be somewhat better in the long run, since
currently these hints will get processed if we do a rebuild-all on a
RestyleTracker other than mPendingRestyles, which can happen if we have
'rem' units and have a root element font size change in the
animation-only update or in mPendingAnimationRestyles.)
The patches in this series refactor the process of rebuilding all style
data (RestyleManager::RebuildAllStyleData and
RestyleManager::DoRebuildAllStyleData) so that the process of rebuilding
all style data uses the existing restyle processing loops in
ProcessPendingRestyles. (Rebuilding all style data is what we do when
we need to throw away the rule tree because something has invalidated
the cached data in it.) This removes (increasing, especially with bug
960465 coming) code duplicated between the two codepaths, fixes some
omissions from the separate rebuild-all codepath, and (more immediately)
allows fixing lifetime issues of ReframingStyleContexts objects in bug
1110277 so that we can have a single ReframingStyleContexts for all of
the restyle processing in each restyle processing operation. In other
words, the goal is to change the rebuild-all process from a separate
codepath to a few variables that modify the way ProcessPendingRestyles
works (and make it do the extra work).
This is just a small first step in that process, which moves one piece
of code from a chunk of duplicated and to-be-removed code into a chunk
of code that will be preserved.
This means that we avoid an O(N^2) calling pattern of IsVisible when we
call IsVisible for all tabs of a tabbrowser, since in a tabbrowser
(which uses an nsDeckFrame), IsVisibleConsideringAncestors is O(N) in
number of earlier tabs.
The change to GetAfterFrameForContent prevents the reframe that is part
of the chain of events leading to this bug, and thus fixes the bug on
its own. The change to GetBeforeFrameForContent seems desirable for
symmetry.
Note that patch 6 also independently fixes the reported bug.
This probably needs somewhat careful review. We should examine:
(1) what the rules for calling nsLayoutUtils::GetBeforeFrame and
nsLayoutUtils::GetAfterFrame are, and whether both (or neither)
need to be patched.
(2) What the rules are for which frame the GenConProperty() lives on,
and whether we should adjust nsIFrame::GetGenConPseudos() to either
do something more intelligent, or assert about callers.
(We should probably clean up some of these things in a followup bug.)
Since the symptom of this bug is (once patch 4 is in the tree) only
causing extra reframes, it can only be tested using the new API (from
bug 1115691) for observing reframes. I confirmed that the test for this
bug fails without the patch and passes with the patch (as noted by the
removal of its todo annotation).
This patch fixes the assertion on layout/generic/crashtests/600100.xhtml,
though I haven't investigated why.
This patch is not needed to fix the bug, but it seems like it's probably
desirable. It's not needed for this bug because
MaybeReframeForBeforePseudo and MaybeReframeForAfterPseudo are already
called (by ElementRestyler::RestyleChildren) on only the first and last
continuation or ib-split sibling with the same style. So this patch
should only actually change anything for cases like a block-in-inline
split whose initial inline part is inside of a ::first-line (where
different parts of the block-in-inline split chain have different style).
Since the symptom of this bug is (once patch 6 is in the tree) only
causing extra reframes, it can only be tested using the new API (from
bug 1115691) for observing reframes. I confirmed that the test for this
bug fails without the patch and passes with the patch (as noted by the
removal of its todo annotation).
This patch switches on the new, "actually start the player in the next refresh
driver tick" behavior. It updates PendingPlayerTracker, adding
a StartPendingPlayersOnNextTick method which calls the appropriate method on
AnimationPlayer. The existing StartPendingPlayers is renamed to
StartPendingPlayersNow and is used for testing only.
Furthermore, since we now expect AnimationPlayer::StartOnNextTick to be
functional, AnimationPlayer::DoPlay is updated to use it when there is no
document available. This should make playing an animation player always
asynchronous, that is, always transition to the pending state temporarily
(unless we are already playing).
The function IsSubjectToAsyncTransforms() was not correctly reporting when a
layer was actually subject to async transforms. As a result, sometimes display
items from above such a layer would end up in a layer below. This could
manifest as page elements being improperly covered by async scrolling elements.
We want to time animations from when their first frame is painted. However,
interruptible reflow complicates this since, for a given set of pending
animations, some may be painted whilst others are not. To simplify this we
simply force an uninterruptible reflow when we have animations that are
waiting to start.
Infallible new ensures that |item| is always non-null. And even if it didn't,
AppendItem() dereferences |item| before this code is reached.
--HG--
extra : rebase_source : 9abb8704ba03f455d6b77c5735fcb6cde4f8fef8
We are white-listing the existing set of tests that use setTimeout
like this. Hopefully these tests will be investigated and fixed
in the future, so that we can narrow down the white-list.
This check is only turned on for mochitest-plain for now.
Add a selection state "updateposition" and a field "visible" to indicate
that the current selection's boundingClientRect or visible is changed.
We dispatch this state after scrolling or reflowing is done.
Make DispatchSelectionStateChangedEvent() and GetSelectionBoundingRect()
become member functions of SeletionCarets so that they are easier to use
in later patches.
We only ever look at the bounds of this region so we can just track
that in a single rect instead of a region.
--HG--
extra : rebase_source : 4cd84ab724d9dde1c578e71362cab8033af7a210
