Closed Bug 1341648 Opened 6 years ago Closed 6 years ago

stylo: need to port HTMLTableCellElement::WalkContentStyleRules to stylo

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
We don't seem to have a thing for it right now.
This causes reftest failures at least in layout/reftests/pagination.
Blocks: stylo-reftest
Similar to bug 1341647, over to Manish.
Flags: needinfo?(manishearth)
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Flags: needinfo?(manishearth)
Priority: -- → P1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
splitting out the patches on bug 1341714
Blocks: 1341714
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
r?
Flags: needinfo?(bzbarsky)
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-
Addressed. Bug 1341647 handles posting restyle updates for all other mapped attributes, but I included the cellpadding one here.
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+
Flags: needinfo?(bzbarsky)
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
Split out servo bits into https://github.com/servo/servo/pull/16199
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5309c052d43e
stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ; r=bz
https://hg.mozilla.org/mozilla-central/rev/5309c052d43e
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.