stylo: need to port HTMLTableCellElement::WalkContentStyleRules to stylo

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: bz, Assigned: manishearth)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

We don't seem to have a thing for it right now.
This causes reftest failures at least in layout/reftests/pagination.
Blocks: 1324348
Similar to bug 1341647, over to Manish.
Flags: needinfo?(manishearth)
(Assignee)

Updated

5 months ago
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Flags: needinfo?(manishearth)
Priority: -- → P1
(Assignee)

Updated

5 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1341714
(Assignee)

Comment 4

5 months ago
splitting out the patches on bug 1341714
Blocks: 1341714
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment hidden (mozreview-request)
(Assignee)

Comment 6

5 months ago
r?
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 7

5 months ago
mozreview-review
Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

https://reviewboard.mozilla.org/r/124788/#review127440

This doesn't handle dynamic changes to cellpadding correctly, as evidenced by the fact that we're still failing the 586400-1.html test in layout/reftests/bugs/reftest-stylo.list

Gecko handles this by having nsHTMLStyleSheet::HasAttributeDependentStyle explicitly return eRestyle_Subtree for the case of a cellpadding attribute changing on a <table> element.  We need some equivalent for stylo.

::: dom/html/HTMLTableCellElement.h:152
(Diff revision 1)
>                                nsAttrValue& aResult) override;
>    virtual nsMapRuleToAttributesFunc GetAttributeMappingFunction() const override;
>    NS_IMETHOD WalkContentStyleRules(nsRuleWalker* aRuleWalker) override;
>    NS_IMETHOD_(bool) IsAttributeMapped(const nsIAtom* aAttribute) const override;
> +  // Get mapped attributes of parent table, if any
> +  nsMappedAttributes* GetParentMappedAttributes() const;

GetTableMappedAttributes, please.  It's not really our "parent" normally.  Or maybe GetMappedAttributesInheritedFromTable?

::: dom/html/HTMLTableElement.cpp:866
(Diff revision 1)
>    if (mTableInheritedAttributes) {
> -    if (mTableInheritedAttributes == TABLE_ATTRS_DIRTY)
> -      BuildInheritedAttributes();
> -    if (mTableInheritedAttributes != TABLE_ATTRS_DIRTY)
>        return mTableInheritedAttributes;
>    }
>    return nullptr;

This looks like a complicated way to write:

  return mTableInheritedAttributes;

::: dom/html/HTMLTableElement.cpp:901
(Diff revision 1)
>          // Reset the stylesheet of modifiableMapped so that it doesn't
>          // spend time trying to remove itself from the hash.  There is no
>          // risk that modifiableMapped is in the hash since we created
>          // it ourselves and it didn't come from the stylesheet (in which
>          // case it would not have been modifiable).
> +

Why this extra blank line?

::: dom/html/HTMLTableElement.cpp:913
(Diff revision 1)
> -  if (mTableInheritedAttributes &&
> +  if (mTableInheritedAttributes)
> -      mTableInheritedAttributes != TABLE_ATTRS_DIRTY)
>      NS_RELEASE(mTableInheritedAttributes);
> -  mTableInheritedAttributes = TABLE_ATTRS_DIRTY;
> +  mTableInheritedAttributes = nullptr;

Just NS_IF_RELEASE(mTableInheritedAttributes) here.  And note that that nulls it out too, so no need to assign nullptr.

::: layout/style/ServoBindings.h:194
(Diff revision 1)
>  RawServoDeclarationBlockStrongBorrowedOrNull
>  Gecko_GetStyleAttrDeclarationBlock(RawGeckoElementBorrowed element);
>  RawServoDeclarationBlockStrongBorrowedOrNull
>  Gecko_GetHTMLPresentationAttrDeclarationBlock(RawGeckoElementBorrowed element);
> +RawServoDeclarationBlockStrongBorrowedOrNull
> +Gecko_GetExtraContentStyleRules(RawGeckoElementBorrowed element);

Gecko_GetExtraContentStyleDeclarations, no?

::: layout/style/ServoBindings.cpp:409
(Diff revision 1)
> +Gecko_GetExtraContentStyleRules(RawGeckoElementBorrowed aElement)
> +{
> +  static_assert(sizeof(RefPtr<RawServoDeclarationBlock>) ==
> +                sizeof(RawServoDeclarationBlockStrong),
> +                "RefPtr should just be a pointer");
> +  if (aElement->IsHTMLElement(nsGkAtoms::td) || aElement->IsHTMLElement(nsGkAtoms::th)) {

This should probably be aElement->IsAnyOfHTMLElements(nsGkAtoms::td, nsGkAtoms:th) to avoid doing the namespace check twice.

Also, why not invert the condition and early return if not one of those elements, then outdent everything.
Attachment #8852600 - Flags: review-
(Assignee)

Comment 8

5 months ago
Addressed. Bug 1341647 handles posting restyle updates for all other mapped attributes, but I included the cellpadding one here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 13

5 months ago
mozreview-review
Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

https://reviewboard.mozilla.org/r/124788/#review127862

::: layout/reftests/table-anonymous-boxes/reftest-stylo.list:50
(Diff revisions 1 - 4)
> -fails == 3-tables-ref.html 3-tables-ref.html # Bug 1341775
> -fails == 3-tables-ref.html 3-tables-ref.html # Bug 1341775
> +== 3-tables-ref.html 3-tables-ref.html # Bug 1341775
> +== 3-tables-ref.html 3-tables-ref.html # Bug 1341775

The "Bug 1341775" comments here should go away.

::: layout/style/ServoBindings.cpp:414
(Diff revisions 1 - 4)
> -      const RefPtr<RawServoDeclarationBlock>& servo = attrs->GetServoStyle();
> +    const RefPtr<RawServoDeclarationBlock>& servo = attrs->GetServoStyle();
> -      return reinterpret_cast<const RawServoDeclarationBlockStrong*>(&servo);
> +    return reinterpret_cast<const RawServoDeclarationBlockStrong*>(&servo);

This bit is now duplicated with Gecko_GetHTMLPresentationAttrDeclarationBlock.  Might be worth pushing it down into a method on nsMappedAttributes that's called from both places....  Followup probably best for this.

::: layout/base/ServoRestyleManager.cpp:529
(Diff revision 4)
>    MOZ_ASSERT_IF(snapshot, snapshot->HasAttrs());
>  #endif
>    if (aAttribute == nsGkAtoms::style) {
>      PostRestyleEvent(aElement, eRestyle_StyleAttribute, nsChangeHint(0));
>    }
> +  if (aAttribute == nsGkAtoms::cellpadding && aElement->IsHTMLElement(nsGkAtoms::table)) {

This could use a comment about how this gets mapped into style for the table's cells, hence we need to restyle those cells.
Attachment #8852600 - Flags: review?(bzbarsky) → review+
(Reporter)

Updated

5 months ago
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 14

5 months ago
mozreview-review-reply
Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

https://reviewboard.mozilla.org/r/124788/#review127862

> This bit is now duplicated with Gecko_GetHTMLPresentationAttrDeclarationBlock.  Might be worth pushing it down into a method on nsMappedAttributes that's called from both places....  Followup probably best for this.

bug 1352293, will clean up once this trio of bugs lands
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

5 months ago
Split out servo bits into https://github.com/servo/servo/pull/16199

Comment 19

5 months ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5309c052d43e
stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ; r=bz

Comment 20

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5309c052d43e
Status: REOPENED → RESOLVED
Last Resolved: 5 months ago5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.