Bug 1572246 - Make <link> always unvisited. r=bzbarsky
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 15 Aug 2019 14:10:07 +0000
changeset 488235 b3194a6ba51e91aefe3933da163e6ba622b0c9f7
parent 488234 8bc7660d5f16c2f14fa673bac5a7d178a48b48c8
child 488236 31defc04f73a3e33c4b279fc317953614c953cbb
push id113904
push userncsoregi@mozilla.com
push dateThu, 15 Aug 2019 19:41:00 +0000
treeherdermozilla-inbound@b283a7ef186c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs1572246
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1572246 - Make <link> always unvisited. r=bzbarsky Intent email: https://groups.google.com/d/msg/mozilla.dev.platform/1NP6oJzK6zg/ftAz_TajAAAJ For now do the obvious check rather than bigger refactorings, since we keep them matching :link or not depending on whether they have an href. I'll file an HTML spec issue about not making them traversable, and a MathML issue about the craziness that it is that almost all MathML elements can be links. Differential Revision: https://phabricator.services.mozilla.com/D41269
dom/base/Link.cpp
dom/html/HTMLLinkElement.cpp
dom/html/HTMLLinkElement.h
layout/reftests/css-visited/color-on-htmllinkelement-1-ref.html
layout/reftests/css-visited/color-on-htmllinkelement-1.html
layout/style/test/moz.build
layout/style/test/test_visited_reftests.html
--- a/dom/base/Link.cpp
+++ b/dom/base/Link.cpp
@@ -321,19 +321,18 @@ void Link::CancelPrefetchOrPreload() {
     nsCOMPtr<nsIURI> uri(GetURI());
     if (uri) {
       prefetchService->CancelPrefetchPreloadURI(uri, mElement);
     }
   }
 }
 
 void Link::SetLinkState(nsLinkState aState) {
-  NS_ASSERTION(mRegistered, "Setting the link state of an unregistered Link!");
-  NS_ASSERTION(mLinkState != aState,
-               "Setting state to the currently set state!");
+  MOZ_ASSERT(mRegistered, "Setting the link state of an unregistered Link!");
+  MOZ_ASSERT(mLinkState != aState, "Setting state to the currently set state!");
 
   // Set our current state as appropriate.
   mLinkState = aState;
 
   // Per IHistory interface documentation, we are no longer registered.
   mRegistered = false;
 
   MOZ_ASSERT(LinkState() == NS_EVENT_STATE_VISITED ||
@@ -342,16 +341,18 @@ void Link::SetLinkState(nsLinkState aSta
 
   // Tell the element to update its visited state
   mElement->UpdateState(true);
 }
 
 EventStates Link::LinkState() const {
   // We are a constant method, but we are just lazily doing things and have to
   // track that state.  Cast away that constness!
+  //
+  // XXX(emilio): that's evil.
   Link* self = const_cast<Link*>(this);
 
   Element* element = self->mElement;
 
   // If we have not yet registered for notifications and need to,
   // due to our href changing, register now!
   if (!mRegistered && mNeedsRegistration && element->IsInComposedDoc() &&
       !HasPendingLinkUpdate()) {
@@ -742,18 +743,22 @@ void Link::ResetLinkState(bool aNotify, 
     Document* doc = mElement->GetComposedDoc();
     if (doc && (mRegistered || mLinkState == eLinkState_Visited)) {
       // Tell the document to forget about this link if we've registered
       // with it before.
       doc->ForgetLink(this);
     }
   }
 
-  // If we have an href, we should register with the history.
-  mNeedsRegistration = aHasHref;
+  // If we have an href, and we're not a <link>, we should register with the
+  // history.
+  //
+  // FIXME(emilio): Do we really want to allow all MathML elements to be
+  // :visited? That seems not great.
+  mNeedsRegistration = aHasHref && !mElement->IsHTMLElement(nsGkAtoms::link);
 
   // If we've cached the URI, reset always invalidates it.
   UnregisterFromHistory();
   mCachedURI = nullptr;
 
   // Update our state back to the default.
   mLinkState = defaultState;
 
--- a/dom/html/HTMLLinkElement.cpp
+++ b/dom/html/HTMLLinkElement.cpp
@@ -119,19 +119,16 @@ bool HTMLLinkElement::HasDeferredDNSPref
 
 nsresult HTMLLinkElement::BindToTree(BindContext& aContext, nsINode& aParent) {
   Link::ResetLinkState(false, Link::ElementHasHref());
 
   nsresult rv = nsGenericHTMLElement::BindToTree(aContext, aParent);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (IsInComposedDoc()) {
-    if (!aContext.OwnerDoc().NodePrincipal()->IsSystemPrincipal()) {
-      aContext.OwnerDoc().RegisterPendingLinkUpdate(this);
-    }
     TryDNSPrefetchOrPreconnectOrPrefetchOrPreloadOrPrerender();
   }
 
   void (HTMLLinkElement::*update)() =
       &HTMLLinkElement::UpdateStyleSheetInternal;
   nsContentUtils::AddScriptRunner(
       NewRunnableMethod("dom::HTMLLinkElement::BindToTree", this, update));
 
--- a/dom/html/HTMLLinkElement.h
+++ b/dom/html/HTMLLinkElement.h
@@ -12,16 +12,18 @@
 #include "nsGenericHTMLElement.h"
 #include "nsStyleLinkElement.h"
 
 namespace mozilla {
 class EventChainPostVisitor;
 class EventChainPreVisitor;
 namespace dom {
 
+// NOTE(emilio): If we stop inheriting from Link, we need to remove the
+// IsHTMLElement(nsGkAtoms::link) checks in Link.cpp.
 class HTMLLinkElement final : public nsGenericHTMLElement,
                               public nsStyleLinkElement,
                               public Link {
  public:
   explicit HTMLLinkElement(
       already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo);
 
   // nsISupports
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-visited/color-on-htmllinkelement-1-ref.html
@@ -0,0 +1,7 @@
+<!doctype html>
+<style>
+link { display: block; width: 100px; height: 100px; background: currentcolor; }
+link { color: fuchsia; }
+</style>
+<body>
+<link href="visited-page.html">
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-visited/color-on-htmllinkelement-1.html
@@ -0,0 +1,8 @@
+<!doctype html>
+<style>
+link { display: block; width: 100px; height: 100px; background: currentcolor; }
+:link { color: fuchsia; }
+:visited { color: purple; }
+</style>
+<body>
+<link href="visited-page.html">
--- a/layout/style/test/moz.build
+++ b/layout/style/test/moz.build
@@ -48,16 +48,18 @@ TEST_HARNESS_FILES.testing.mochitest.tes
     '/layout/reftests/css-visited/border-collapse-1-ref.html',
     '/layout/reftests/css-visited/border-collapse-1.html',
     '/layout/reftests/css-visited/caret-color-on-visited-1-ref.html',
     '/layout/reftests/css-visited/caret-color-on-visited-1.html',
     '/layout/reftests/css-visited/color-choice-1-ref.html',
     '/layout/reftests/css-visited/color-choice-1.html',
     '/layout/reftests/css-visited/color-on-bullets-1-ref.html',
     '/layout/reftests/css-visited/color-on-bullets-1.html',
+    '/layout/reftests/css-visited/color-on-htmllinkelement-1-ref.html',
+    '/layout/reftests/css-visited/color-on-htmllinkelement-1.html',
     '/layout/reftests/css-visited/color-on-link-1-ref.html',
     '/layout/reftests/css-visited/color-on-link-1.html',
     '/layout/reftests/css-visited/color-on-link-before-1.html',
     '/layout/reftests/css-visited/color-on-text-decoration-1-ref.html',
     '/layout/reftests/css-visited/color-on-text-decoration-1.html',
     '/layout/reftests/css-visited/color-on-visited-1-ref.html',
     '/layout/reftests/css-visited/color-on-visited-1.html',
     '/layout/reftests/css-visited/color-on-visited-before-1.html',
--- a/layout/style/test/test_visited_reftests.html
+++ b/layout/style/test/test_visited_reftests.html
@@ -90,16 +90,18 @@ var gTests = [
   "== mathml-links.html mathml-links-ref.html",
   "== placeholder-1.html placeholder-1-ref.html",
   "== visited-inherit-1.html visited-inherit-1-ref.html",
   "== transition-on-visited.html transition-on-visited-ref.html",
   "== logical-box-border-color-visited-link-001.html logical-box-border-color-visited-link-ref.html",
   "== logical-box-border-color-visited-link-002.html logical-box-border-color-visited-link-ref.html",
   "== logical-box-border-color-visited-link-003.html logical-box-border-color-visited-link-ref.html",
   "== svg-paint-currentcolor-visited.svg svg-paint-currentcolor-visited-ref.svg",
+
+  "== color-on-htmllinkelement-1.html color-on-htmllinkelement-1-ref.html",
 ];
 
 // We record the maximum number of times we had to look at a test before
 // it switched to the passing state (though we assume it's 10 to start
 // rather than 0 so that we have a reasonable default).  Then we make a
 // test "time out" if it takes more than gTimeoutFactor times that
 // amount of time.  This allows us to report a test failure rather than
 // making a test failure just show up as a timeout.