Add support for using appearance:base [chromium/src : main]
David Baron (Gerrit)
David Baron voted and added 8 comments![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
8 comments
This patch add some support for using appearance:base in addition to```suggestion
This patch adds some support for using appearance:base in addition to
```
elements when the flag is enabled.Maybe mention earlier than this that the whole thing is behind a flag?
if (select && !select->SupportsBaseAppearance()) {
builder.SetInBaseSelectAppearance(false);
} else {
builder.SetInBaseSelectAppearance(true);
}The old code here seems wrong when `select` is null. I think the new code makes sense -- but is there anything to be careful of because you're fixing/changing this?
virtual bool SupportsBaseAppearanceInternal(BaseAppearanceValue) const {should `SupportsBaseAppearanceInternal` be in the `protected:` section?
static HTMLSelectElement* GetSelectForPopoverPickerElement(const Element*);I think you've removed every use of `GetSelectForPopoverPickerElement` so you should remove it.
(RuntimeEnabledFeatures::AppearanceBaseEnabled() &&Is a flag check really needed here? I'd think maybe a `DCHECK()` would be sufficient, since an unsupported value shouldn't get through to here, I think.
(RuntimeEnabledFeatures::AppearanceBaseEnabled() &&same here about flag checks
Related details
- Joey Arhar
- Luke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 7 comments![]( "Open in Gerrit")
This patch add some support for using appearance:base in addition to```suggestion
This patch adds some support for using appearance:base in addition to
```
Done
Maybe mention earlier than this that the whole thing is behind a flag?
Done
if (select && !select->SupportsBaseAppearance()) {
builder.SetInBaseSelectAppearance(false);
} else {
builder.SetInBaseSelectAppearance(true);
}The old code here seems wrong when `select` is null. I think the new code makes sense -- but is there anything to be careful of because you're fixing/changing this?
Good catch. This change likely fixes the rendering of the following example when CustomizableSelectInPage is enabled:
```
data:text/html,<select multiple><option>normal</option><div style="appearance:base-select"><option style="appearance:base-select"><div>div</div><span>span</span>text
```
I could add a reftest for it if you think its worth testing.
virtual bool SupportsBaseAppearanceInternal(BaseAppearanceValue) const {should `SupportsBaseAppearanceInternal` be in the `protected:` section?
Done
static HTMLSelectElement* GetSelectForPopoverPickerElement(const Element*);I think you've removed every use of `GetSelectForPopoverPickerElement` so you should remove it.
Done
Is a flag check really needed here? I'd think maybe a `DCHECK()` would be sufficient, since an unsupported value shouldn't get through to here, I think.
Done
same here about flag checks
- David Baron
- Luke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |