Bug 255844 – Add padding:1px to <td> and <th> outside tables, per spec

• Component: CSS
• Status: NEW

[Ahmad Saleem]

Hi Team,

While going through Blink's commit, I came across following UA Stylesheet update commit:

Commit - https://chromium.googlesource.com/chromium/src/+/c5ecd6d100aeefb1b77c310beb11e458fc2d139c

Failing WPT Test Case - http://wpt.live/css/css-tables/html-to-css-mapping-1.html

Web-Spec: https://html.spec.whatwg.org/#tables-2

Our UA Stylesheet does not have following:

td, th { padding: 1px; }

^ Adding this manually in the local branch fixed the failing sub-test in WPT testcase but I don't know whether it would've any unintended effect.

Blink added this with different rule in their stylesheet because they had some styling enforced via HTMLTableCellElement::AdditionalPresentationAttributeStyle() function.

I didn't find any special styling within WebKit in HTMLTableCellElement though.

Appreciate if someone can confirm, if my understanding is right and I am happy to do PR.

Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/108716695>

[Alexsander Borges Damaceno]

Pull request: https://github.com/WebKit/WebKit/pull/37269

[Karl Dubost]

https://searchfox.org/wubkat/rev/47b66cb0dc7c2146fa7576555c720080708667f3/Source/WebCore/html/HTMLTableCellElement.cpp#92-123
```cpp
{
    switch (name.nodeName()) {
    case AttributeNames::nowrapAttr:
    case AttributeNames::widthAttr:
    case AttributeNames::heightAttr:
        return true;
    default:
        break;
    }
    return HTMLTablePartElement::hasPresentationalHintsForAttribute(name);
}

void HTMLTableCellElement::collectPresentationalHintsForAttribute(const QualifiedName& name, const AtomString& value, MutableStyleProperties& style)
{
    switch (name.nodeName()) {
    case AttributeNames::nowrapAttr:
        addPropertyToPresentationalHintStyle(style, CSSPropertyWhiteSpaceCollapse, CSSValueCollapse);
        addPropertyToPresentationalHintStyle(style, CSSPropertyTextWrapMode, CSSValueNowrap);
        break;
    case AttributeNames::widthAttr:
        // width="0" is not allowed for compatibility with WinIE.
        addHTMLLengthToStyle(style, CSSPropertyWidth, value, AllowZeroValue::No);
        break;
    case AttributeNames::heightAttr:
        // width="0" is not allowed for compatibility with WinIE.
        addHTMLLengthToStyle(style, CSSPropertyHeight, value, AllowZeroValue::No);
        break;
    default:
        HTMLTablePartElement::collectPresentationalHintsForAttribute(name, value, style);
        break;
    }
}
```

[Karl Dubost]

Hmm adding this will trigger a huge rebaseligning.  

```
td, th { padding: 1px; }
```

Does it fix a lot of tests?

[Karl Dubost]

In chrome they have. 

const CSSPropertyValueSet*
HTMLTableCellElement::AdditionalPresentationAttributeStyle() {
  if (HTMLTableElement* table = FindParentTable())
    return table->AdditionalCellStyle();
  return nullptr;
}

with 

const CSSPropertyValueSet* HTMLTableElement::AdditionalCellStyle() {
  if (!shared_cell_style_)
    shared_cell_style_ = CreateSharedCellStyle();
  return shared_cell_style_.Get();
}


and

CSSPropertyValueSet* HTMLTableElement::CreateSharedCellStyle() {
  auto* style =
      MakeGarbageCollected<MutableCSSPropertyValueSet>(kHTMLQuirksMode);

  switch (GetCellBorders()) {
    case kSolidBordersColsOnly:
      style->SetLonghandProperty(CSSPropertyID::kBorderLeftWidth,
                                 CSSValueID::kThin);
      style->SetLonghandProperty(CSSPropertyID::kBorderRightWidth,
                                 CSSValueID::kThin);
      style->SetLonghandProperty(CSSPropertyID::kBorderLeftStyle,
                                 CSSValueID::kSolid);
      style->SetLonghandProperty(CSSPropertyID::kBorderRightStyle,
                                 CSSValueID::kSolid);
      style->SetProperty(CSSPropertyID::kBorderColor,
                         *CSSInheritedValue::Create());
      break;
    case kSolidBordersRowsOnly:
      style->SetLonghandProperty(CSSPropertyID::kBorderTopWidth,
                                 CSSValueID::kThin);
      style->SetLonghandProperty(CSSPropertyID::kBorderBottomWidth,
                                 CSSValueID::kThin);
      style->SetLonghandProperty(CSSPropertyID::kBorderTopStyle,
                                 CSSValueID::kSolid);
      style->SetLonghandProperty(CSSPropertyID::kBorderBottomStyle,
                                 CSSValueID::kSolid);
      style->SetProperty(CSSPropertyID::kBorderColor,
                         *CSSInheritedValue::Create());
      break;
    case kSolidBorders:
      style->SetProperty(CSSPropertyID::kBorderWidth,
                         *CSSNumericLiteralValue::Create(
                             1, CSSPrimitiveValue::UnitType::kPixels));
      style->SetProperty(CSSPropertyID::kBorderStyle,
                         *CSSIdentifierValue::Create(CSSValueID::kSolid));
      style->SetProperty(CSSPropertyID::kBorderColor,
                         *CSSInheritedValue::Create());
      break;
    case kInsetBorders:
      style->SetProperty(CSSPropertyID::kBorderWidth,
                         *CSSNumericLiteralValue::Create(
                             1, CSSPrimitiveValue::UnitType::kPixels));
      style->SetProperty(CSSPropertyID::kBorderStyle,
                         *CSSIdentifierValue::Create(CSSValueID::kInset));
      style->SetProperty(CSSPropertyID::kBorderColor,
                         *CSSInheritedValue::Create());
      break;
    case kNoBorders:
      // If 'rules=none' then allow any borders set at cell level to take
      // effect.
      break;
  }

  if (padding_)
    style->SetProperty(CSSPropertyID::kPadding,
                       *CSSNumericLiteralValue::Create(
                           padding_, CSSPrimitiveValue::UnitType::kPixels));

  return style;
}

[Karl Dubost]

A simpler test

1. Go to about:blank
2. Insert the following in the Web Inspector console:
`document.body.appendChild( document.createElement('td') ) && document.body.firstElementChild;`
3. `window.getComputedStyle(document.querySelector('td')).padding`

* Expected: `1px`
* Actual: `0px`

Firefox and Chrome return `1px`
Safari returns `0px`

[Karl Dubost]

Chrome is using a hack to make the WPT test pass, but they didn't really solve the issue in the c++ code. 

```
td:not(table td),
th:not(table th) {
    padding: 1px
}
```

We need to figure out how this is handle in the code.

[Karl Dubost]

Firefox is doing something special about cell padding. 

https://searchfox.org/mozilla-central/rev/a965e3c683ecc035dee1de72bd33a8d91b1203ed/dom/html/HTMLTableElement.cpp#983-995
```cpp
void HTMLTableElement::AfterSetAttr(int32_t aNameSpaceID, nsAtom* aName,
                                    const nsAttrValue* aValue,
                                    const nsAttrValue* aOldValue,
                                    nsIPrincipal* aSubjectPrincipal,
                                    bool aNotify) {
  if (aName == nsGkAtoms::cellpadding && aNameSpaceID == kNameSpaceID_None) {
    BuildInheritedAttributes();
    // This affects our cell styles.
    // TODO(emilio): Maybe GetAttributeChangeHint should also allow you to
    // specify a restyle hint and this could move there?
    nsLayoutUtils::PostRestyleEvent(this, RestyleHint::RestyleSubtree(),
                                    nsChangeHint(0));
  }
```