Bug 1643246 - Don't use attribute selectors for determining if a sele…

…ct is a drop down or a list box. r=emilio Instead add a pseudo-class that does the expected size="" attribute parsing. Removing the Gtk-specific rule setting the text color since it doesn't seem to have any effect currently. Differential Revision: https://phabricator.services.mozilla.com/D83448
dothq · Jul 17, 2020 · 82b32f1 · 82b32f1
1 parent 24d2a26
commit 82b32f1
Show file tree

Hide file tree

Showing 10 changed files with 52 additions and 37 deletions.
diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp
@@ -3201,8 +3201,8 @@ void RestyleManager::ContentStateChanged(nsIContent* aContent,
 
 static inline bool AttributeInfluencesOtherPseudoClassState(
     const Element& aElement, const nsAtom* aAttribute) {
-  // We must record some state for :-moz-browser-frame and
-  // :-moz-table-border-nonzero.
+  // We must record some state for :-moz-browser-frame,
+  // :-moz-table-border-nonzero, and :-moz-select-list-box.
   if (aAttribute == nsGkAtoms::mozbrowser) {
     return aElement.IsAnyOfHTMLElements(nsGkAtoms::iframe, nsGkAtoms::frame);
   }
@@ -3211,6 +3211,10 @@ static inline bool AttributeInfluencesOtherPseudoClassState(
     return aElement.IsHTMLElement(nsGkAtoms::table);
   }
 
+  if (aAttribute == nsGkAtoms::multiple || aAttribute == nsGkAtoms::size) {
+    return aElement.IsHTMLElement(nsGkAtoms::select);
+  }
+
   return false;
 }
 

diff --git a/layout/style/GeckoBindings.cpp b/layout/style/GeckoBindings.cpp
@@ -68,6 +68,7 @@
 #include "mozilla/dom/ElementInlines.h"
 #include "mozilla/dom/HTMLTableCellElement.h"
 #include "mozilla/dom/HTMLBodyElement.h"
+#include "mozilla/dom/HTMLSelectElement.h"
 #include "mozilla/dom/HTMLSlotElement.h"
 #include "mozilla/dom/MediaList.h"
 #include "mozilla/dom/SVGElement.h"
@@ -774,6 +775,11 @@ bool Gecko_IsBrowserFrame(const Element* aElement) {
   return browserFrame && browserFrame->GetReallyIsBrowser();
 }
 
+bool Gecko_IsSelectListBox(const Element* aElement) {
+  const auto* select = HTMLSelectElement::FromNode(aElement);
+  return select && !select->IsCombobox();
+}
+
 template <typename Implementor>
 static nsAtom* LangValue(Implementor* aElement) {
   // TODO(emilio): Deduplicate a bit with nsIContent::GetLang().

diff --git a/layout/style/GeckoBindings.h b/layout/style/GeckoBindings.h
@@ -147,6 +147,7 @@ const mozilla::PreferenceSheet::Prefs* Gecko_GetPrefSheetPrefs(
 
 bool Gecko_IsTableBorderNonzero(const mozilla::dom::Element* element);
 bool Gecko_IsBrowserFrame(const mozilla::dom::Element* element);
+bool Gecko_IsSelectListBox(const mozilla::dom::Element* element);
 
 // Attributes.
 #define SERVO_DECLARE_ELEMENT_ATTR_MATCHING_FUNCTIONS(prefix_, implementor_)   \

diff --git a/layout/style/ServoElementSnapshot.cpp b/layout/style/ServoElementSnapshot.cpp
@@ -17,6 +17,7 @@ ServoElementSnapshot::ServoElementSnapshot(const Element& aElement)
       mContains(Flags(0)),
       mIsTableBorderNonzero(false),
       mIsMozBrowserFrame(false),
+      mIsSelectListBox(false),
       mClassAttributeChanged(false),
       mIdAttributeChanged(false) {
   MOZ_COUNT_CTOR(ServoElementSnapshot);
@@ -32,6 +33,7 @@ void ServoElementSnapshot::AddOtherPseudoClassState(const Element& aElement) {
 
   mIsTableBorderNonzero = Gecko_IsTableBorderNonzero(&aElement);
   mIsMozBrowserFrame = Gecko_IsBrowserFrame(&aElement);
+  mIsSelectListBox = Gecko_IsSelectListBox(&aElement);
 
   mContains |= Flags::OtherPseudoClassState;
 }

diff --git a/layout/style/ServoElementSnapshot.h b/layout/style/ServoElementSnapshot.h
@@ -142,6 +142,11 @@ class ServoElementSnapshot {
     return mIsMozBrowserFrame;
   }
 
+  bool IsSelectListBox() const {
+    MOZ_ASSERT(HasOtherPseudoClassState());
+    return mIsSelectListBox;
+  }
+
  private:
   // TODO: Profile, a 1 or 2 element AutoTArray could be worth it, given we know
   // we're dealing with attribute changes when we take snapshots of attributes,
@@ -156,6 +161,7 @@ class ServoElementSnapshot {
   bool mSupportsLangAttr : 1;
   bool mIsTableBorderNonzero : 1;
   bool mIsMozBrowserFrame : 1;
+  bool mIsSelectListBox : 1;
   bool mClassAttributeChanged : 1;
   bool mIdAttributeChanged : 1;
 };

diff --git a/layout/style/res/forms.css b/layout/style/res/forms.css
@@ -220,62 +220,41 @@ textarea:read-write {
 select {
   margin: 0;
   border-color: ThreeDLightShadow;
-  background-color: -moz-Combobox;
-  color: -moz-ComboboxText;
   font: -moz-list;
   white-space: nowrap !important;
   word-wrap: normal !important;
   cursor: default;
   box-sizing: border-box;
   user-select: none;
-  appearance: auto;
-  -moz-default-appearance: menulist;
   border-width: 2px;
   border-style: inset;
   overflow: -moz-hidden-unscrollable;
   /* No text-decoration reaching inside, by default */
   display: inline-block;
   page-break-inside: avoid;
   overflow-clip-box: padding-box !important; /* bug 992447 */
-}
 
-%ifdef MOZ_WIDGET_GTK
-/* Comboboxes use button styles on Gtk so it's safe to
- * use a button style here. */
-select:active:hover {
-  color: -moz-gtk-buttonactivetext;
+  /* Set some styles for drop down selects. These are overridden below for
+   * list box selects. */
+  background-color: -moz-Combobox;
+  color: -moz-ComboboxText;
+  vertical-align: baseline;
+  padding-block: 0;
+  appearance: auto;
+  -moz-default-appearance: menulist;
+  -moz-button-appearance: allow;
 }
-%endif
-
-/* Need the "select[size][multiple]" selector to override the settings on
-   'select[size="1"]', eg if one has <select size="1" multiple>, and that it
-   needs to be separate from the other :is() selector because otherwise it'd
-   increase the specificity of it, and override the rule below */
 
-select:is([size], [multiple]),
-select[size][multiple] {
-  /* Different alignment and padding for listbox vs combobox */
+select:-moz-select-list-box {
   background-color: -moz-Field;
   color: -moz-FieldText;
-  padding-block: 1px;
-  padding-inline: 0;
   vertical-align: text-bottom;
+  padding-block: 1px;
   appearance: auto;
   -moz-default-appearance: listbox;
   -moz-button-appearance: disallow;
 }
 
-select:is([size="0"], [size="1"]) {
-  /* Except this is not a listbox */
-  background-color: -moz-Combobox;
-  color: -moz-ComboboxText;
-  vertical-align: baseline;
-  padding: unset;
-  appearance: auto;
-  -moz-default-appearance: menulist;
-  -moz-button-appearance: allow;
-}
-
 select > button {
   inline-size: 12px;
   white-space: nowrap;
@@ -431,9 +410,7 @@ input:disabled,
 textarea:disabled,
 option:disabled,
 optgroup:disabled,
-select:disabled:disabled /* Need the pseudo-class twice to have the specificity
-                            be at least the same as select[size][multiple] above */
-{
+select:disabled {
   color: GrayText;
   background-color: ThreeDLightShadow;
   cursor: unset;

diff --git a/servo/components/style/gecko/non_ts_pseudo_class_list.rs b/servo/components/style/gecko/non_ts_pseudo_class_list.rs
@@ -32,6 +32,7 @@ macro_rules! apply_non_ts_list {
             [
                 ("-moz-table-border-nonzero", MozTableBorderNonzero, _, PSEUDO_CLASS_ENABLED_IN_UA_SHEETS),
                 ("-moz-browser-frame", MozBrowserFrame, _, PSEUDO_CLASS_ENABLED_IN_UA_SHEETS_AND_CHROME),
+                ("-moz-select-list-box", MozSelectListBox, _, PSEUDO_CLASS_ENABLED_IN_UA_SHEETS),
                 ("link", Link, IN_UNVISITED_STATE, _),
                 ("any-link", AnyLink, IN_VISITED_OR_UNVISITED_STATE, _),
                 ("visited", Visited, IN_VISITED_STATE, _),

diff --git a/servo/components/style/gecko/wrapper.rs b/servo/components/style/gecko/wrapper.rs
@@ -2110,6 +2110,9 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
                 bindings::Gecko_IsTableBorderNonzero(self.0)
             },
             NonTSPseudoClass::MozBrowserFrame => unsafe { bindings::Gecko_IsBrowserFrame(self.0) },
+            NonTSPseudoClass::MozSelectListBox => unsafe {
+                bindings::Gecko_IsSelectListBox(self.0)
+            },
             NonTSPseudoClass::MozIsHTML => self.is_html_element_in_html_document(),
             NonTSPseudoClass::MozLWTheme => self.document_theme() != DocumentTheme::Doc_Theme_None,
             NonTSPseudoClass::MozLWThemeBrightText => {

diff --git a/servo/components/style/invalidation/element/element_wrapper.rs b/servo/components/style/invalidation/element/element_wrapper.rs
@@ -239,6 +239,15 @@ where
                 }
             },
 
+            #[cfg(feature = "gecko")]
+            NonTSPseudoClass::MozSelectListBox => {
+                if let Some(snapshot) = self.snapshot() {
+                    if snapshot.has_other_pseudo_class_state() {
+                        return snapshot.mIsSelectListBox();
+                    }
+                }
+            },
+
             // :lang() needs to match using the closest ancestor xml:lang="" or
             // lang="" attribtue from snapshots.
             NonTSPseudoClass::Lang(ref lang_arg) => {

diff --git a/servo/components/style/invalidation/element/invalidation_map.rs b/servo/components/style/invalidation/element/invalidation_map.rs
@@ -506,6 +506,12 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> {
                     NonTSPseudoClass::MozTableBorderNonzero => local_name!("border"),
                     #[cfg(feature = "gecko")]
                     NonTSPseudoClass::MozBrowserFrame => local_name!("mozbrowser"),
+                    #[cfg(feature = "gecko")]
+                    NonTSPseudoClass::MozSelectListBox => {
+                        // This depends on two attributes.
+                        return self.add_attr_dependency(local_name!("multiple")) &&
+                            self.add_attr_dependency(local_name!("size"));
+                    },
                     NonTSPseudoClass::Lang(..) => local_name!("lang"),
                     _ => return true,
                 };