Closed
Bug 1343078
Opened 8 years ago
Closed 8 years ago
Switch placeholder frames to using a style context that doesn't inherit from anything
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Depends on 1 open bug)
Details
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dbaron
:
review+
|
Details |
This should make bug 1292609 simpler, because we won't need to change the style context of placeholder frames at all.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8842329 [details] Bug 1343078 part 1. Give placeholders and first-letter continuations different kinds of anonymous boxes. https://reviewboard.mozilla.org/r/116204/#review119792 r=dbaron with the following comments ::: layout/style/ServoStyleSet.cpp:260 (Diff revision 1) > +} > + > +already_AddRefed<nsStyleContext> > +ServoStyleSet::ResolveStyleForPlaceholder(nsStyleContext* aParentContext) > +{ > + // The parent context can be null if the placer's element is a root element. placer -> placeholder ::: layout/style/nsCSSAnonBoxList.h:22 (Diff revision 1) > */ > > // OUTPUT_CLASS=nsCSSAnonBoxes > // MACRO_NAME=CSS_ANON_BOX > > -// ::-moz-text and ::-moz-other-non-element are non-elements which no > +// ::-moz-text, ::-moz-placeholder, and ::-moz-first-letter-continuation are :-moz-placeholder-anon-box Although I wonder if :-moz-oof-placeholder is better... ::: layout/style/nsCSSAnonBoxList.h:26 (Diff revision 1) > > -// ::-moz-text and ::-moz-other-non-element are non-elements which no > -// rule will match. > +// ::-moz-text, ::-moz-placeholder, and ::-moz-first-letter-continuation are > +// non-elements which no rule will match. > CSS_ANON_BOX(mozText, ":-moz-text") > -// This anonymous box has two uses: > -// 1. placeholder frames, > +// placeholder frames. Note that :-moz-placeholder is already taken by the > +// corresponding pseudo-element, so we need a different string here. "corresponding" here is misleading; it's for <input type="placeholder"> ::: layout/style/nsCSSAnonBoxList.h:27 (Diff revision 1) > -// ::-moz-text and ::-moz-other-non-element are non-elements which no > -// rule will match. > +// ::-moz-text, ::-moz-placeholder, and ::-moz-first-letter-continuation are > +// non-elements which no rule will match. > CSS_ANON_BOX(mozText, ":-moz-text") > -// This anonymous box has two uses: > -// 1. placeholder frames, > -// 2. nsFirstLetterFrames for content outside the ::first-letter. > +// placeholder frames. Note that :-moz-placeholder is already taken by the > +// corresponding pseudo-element, so we need a different string here. > +CSS_ANON_BOX(placeholder, ":-moz-placeholder-anon-box") I'm a little leery of having these two names be so different. That leads to confusion later. I'd prefer having the nsCSSAnonBoxes member have a name that more closely matches the textual CSS.
Attachment #8842329 -
Flags: review?(dbaron) → review+
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8842330 [details] Bug 1343078 part 2. Restrict the "is a root" blockification behavior in nsStyleContext to non-pseudos and a limited set of anonymous boxes that want the behavior. https://reviewboard.mozilla.org/r/116206/#review119794 ::: layout/style/nsStyleContext.cpp:828 (Diff revision 1) > + const nsStyleDisplay* disp = StyleDisplay(); > + I don't follow why you're moving this up. (Did it belong in another patch?) ::: layout/style/nsStyleContext.cpp:871 (Diff revision 1) > + // inherit from anything. So restrict blockification only to actual > + // elements, the viewport (which should be block anyway, but in SVG > + // document's isn't because we lazy-load ua.css there), and the ::backdrop er, yikes. Does this affect any of the other near-root pseudos? (e.g., :-moz-canvas, :-moz-page, :-moz-pagecontent, :-moz-page-sequence, :-moz-scrolled-content, :-moz-scrolled-canvas, :-moz-scrolled-page-sequence?) Or are they all non-root? (I checked :-moz-viewport-scroll since it seemed most worrying.)
Attachment #8842330 -
Flags: review?(dbaron) → review+
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8842331 [details] Bug 1343078 part 3. Introduce the concept of non-inheriting anon boxes without adding any yet. https://reviewboard.mozilla.org/r/116208/#review119796
Attachment #8842331 -
Flags: review?(dbaron) → review+
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8842332 [details] Bug 1343078 part 4. Add storage for the singleton non-inheriting anon box style contexts on style set. https://reviewboard.mozilla.org/r/116210/#review119798 ::: layout/style/ServoStyleSet.h:269 (Diff revision 1) > nsTArray<RefPtr<ServoStyleSheet>>> mSheets; > int32_t mBatching; > > + // Stores pointers to our cached style contexts for non-inheriting anonymous > + // boxes. > + RefPtr<nsStyleContext> mNonInheritingStyleContexts[static_cast<nsCSSAnonBoxes::NonInheritingBase>(nsCSSAnonBoxes::NonInheriting::_Count)]; does this actually compile when \_Count is 0? (If it doesn't... maybe it doesn't matter?) ::: layout/style/nsStyleSet.cpp:313 (Diff revision 1) > + // Clear our cached style contexts for non-inheriting anonymous boxes. > + ClearNonInheritingStyleContexts(); > + I'm a little disturbed that there's no servo equivalent... ::: layout/style/nsStyleSet.cpp:315 (Diff revision 1) > // held on to after the rule tree has been reconstructed. > PresContext()->PresShell()->ClearArenaRefPtrs(eArenaObjectID_nsStyleContext); > > + // Clear our cached style contexts for non-inheriting anonymous boxes. > + ClearNonInheritingStyleContexts(); > + trailing whitespace ::: layout/style/nsStyleSet.cpp:2631 (Diff revision 1) > +void > +nsStyleSet::ClearNonInheritingStyleContexts() > +{ > + for (RefPtr<nsStyleContext>& ptr : mNonInheritingStyleContexts) { > + ptr = nullptr; > + } trailing whitespace
Attachment #8842332 -
Flags: review?(dbaron) → review+
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8842333 [details] Bug 1343078 part 5. Change the restyle manager to handle style contexts with no parent in ReparentStyleContext (by doing nothing with them). https://reviewboard.mozilla.org/r/116212/#review119800 ::: layout/base/GeckoRestyleManager.cpp:869 (Diff revision 1) > + > + if (!newParentContext && !oldContext->GetParent()) { > + // No need to do anything here. > + return NS_OK; > + } > + Perhaps some of the continuation-related assertions should be retained here? Also perhaps an assertion that there aren't any child frames? Since if we were to use this on something that had child frames, we'd presumably need to go through the code below that traverses the children (which would likely be parented to an ancestor)? Also -- I'm a little disturbed again that there isn't an equivalent change on the stylo side.
Attachment #8842333 -
Flags: review?(dbaron) → review+
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8842334 [details] Bug 1343078 part 6. Change nsFrame::CorrectStyleParentFrame to return null if we're dealing with a non-inheriting anon box. https://reviewboard.mozilla.org/r/116214/#review119802
Attachment #8842334 -
Flags: review?(dbaron) → review+
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8842335 [details] Bug 1343078 part 7. Make placeholders a non-inheriting anon box. https://reviewboard.mozilla.org/r/116216/#review119804 ::: layout/style/ServoStyleSet.cpp:269 (Diff revision 2) > } > > already_AddRefed<nsStyleContext> > -ServoStyleSet::ResolveStyleForPlaceholder(nsStyleContext* aParentContext) > +ServoStyleSet::ResolveStyleForPlaceholder() > { > - // The parent context can be null if the placer's element is a root element. > + RefPtr<nsStyleContext>& existing = Maybe clearer to call this "cache" rather than "existing"? ::: layout/style/nsStyleSet.cpp:1846 (Diff revision 2) > } > > already_AddRefed<nsStyleContext> > -nsStyleSet::ResolveStyleForPlaceholder(nsStyleContext* aParentContext) > +nsStyleSet::ResolveStyleForPlaceholder() > { > - return GetContext(aParentContext, mRuleTree, nullptr, > + RefPtr<nsStyleContext>& existing = same here
Attachment #8842335 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 17•8 years ago
|
||
> placer -> placeholder Er, yes, fixed. > Although I wonder if :-moz-oof-placeholder is better... Good idea. Done. > "corresponding" here is misleading; it's for <input type="placeholder"> > I'm a little leery of having these two names be so different. OK. There is no textual CSS for the placeholder case, but I can certainly make the names more similar. Specifically, this part now says: // placeholder frames for out of flows. Note that :-moz-placeholder is used for // the pseudo-element that represents the placeholder text in <input // placeholder="foo">, so we need a different string here. CSS_ANON_BOX(oofPlaceholder, ":-moz-oof-placeholder") > I don't follow why you're moving this up. Oh, I had added an assert in the mPseudoTag == nsCSSAnonBoxes::viewport block that used the nsStyleDisplay, but that assert turned out to not hold for SVG documents (which is part of what the comment I added is about). I forgot to move it back. Good catch, will do so. > Does this affect any of the other near-root pseudos? They should all be unchanged, I think, since being anon boxes they use their parent's style context as parent, and they always have a parent frame. Also, :-moz-canvas, :-moz-page, :-moz-pagecontent, :-moz-page-sequence all have "display: block !important" in ua.css. :-moz-scrolled-content, :-moz-scrolled-canvas, :-moz-scrolled-page-sequence have display:block without !important... What it _did_ affect was the anonymous content container parented to the canvas frame (the one nsIDocument::InsertAnonymousContent inserts into) _and_ its placeholder. The anon content container itself is "position: absolute", so it didn't matter there, but with this patch its placeholder no longer gets display:block. Which is probably for the better. > does this actually compile when \_Count is 0? It seems to, yes. > I'm a little disturbed that there's no servo equivalent... As well you should be! There should be one. I'm adding one, having it called from ServoStyleSet::Shutdown and from ServoStyleSet::RebuildData, before the Servo_StyleSet_RebuildData call. Good catch! > trailing whitespace Fixed, both places. > Perhaps some of the continuation-related assertions should be retained here? Done, by moving my new block and the existing assert that newParentContext exists to after those assertions. > Also perhaps an assertion that there aren't any child frames? Good idea. Added this right before the early return: #ifdef DEBUG // Make sure we have no children, so we really know there is nothing to do. nsIFrame::ChildListIterator lists(aFrame); for (; !lists.IsDone(); lists.Next()) { MOZ_ASSERT(lists.CurrentList().IsEmpty(), "Failing to reparent style context for child of " "non-inheriting anon box"); } #endif // DEBUG > Also -- I'm a little disturbed again that there isn't an equivalent change on the stylo side. Yeah, the stylo side has no concept of reparenting yet. It will need to grow one for ::first-line support.... > Maybe clearer to call this "cache" rather than "existing"? Agreed, fixed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•8 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e5ddadc9ae6d part 1. Give placeholders and first-letter continuations different kinds of anonymous boxes. r=dbaron https://hg.mozilla.org/integration/autoland/rev/44750ea79b9f part 2. Restrict the "is a root" blockification behavior in nsStyleContext to non-pseudos and a limited set of anonymous boxes that want the behavior. r=dbaron https://hg.mozilla.org/integration/autoland/rev/e1855612a5d4 part 3. Introduce the concept of non-inheriting anon boxes without adding any yet. r=dbaron https://hg.mozilla.org/integration/autoland/rev/6e85a63d117c part 4. Add storage for the singleton non-inheriting anon box style contexts on style set. r=dbaron https://hg.mozilla.org/integration/autoland/rev/c13329ddee05 part 5. Change the restyle manager to handle style contexts with no parent in ReparentStyleContext (by doing nothing with them). r=dbaron https://hg.mozilla.org/integration/autoland/rev/930853cae5a4 part 6. Change nsFrame::CorrectStyleParentFrame to return null if we're dealing with a non-inheriting anon box. r=dbaron https://hg.mozilla.org/integration/autoland/rev/afd58f4674d1 part 7. Make placeholders a non-inheriting anon box. r=dbaron
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e5ddadc9ae6d https://hg.mozilla.org/mozilla-central/rev/44750ea79b9f https://hg.mozilla.org/mozilla-central/rev/e1855612a5d4 https://hg.mozilla.org/mozilla-central/rev/6e85a63d117c https://hg.mozilla.org/mozilla-central/rev/c13329ddee05 https://hg.mozilla.org/mozilla-central/rev/930853cae5a4 https://hg.mozilla.org/mozilla-central/rev/afd58f4674d1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•