1530211 - Autofill addresses learn more link is not blue

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Asa Dotzler [:asa]]

Attachment: image.png

The learn more link for autofill addresses implemented in bug 1386922 isn't blue like other links in the preferences panel.

Comment 1

[Matthew N. [:MattN]]

I would guess this is a de-XBL regression from bug 1527495. Since this is the second regression from that bug with the same cause, maybe all the changes in that bug should be audited?

https://phabricator.services.mozilla.com/D19595#change-6AYEJQKqnxaM

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Formautofill sets className which overrides text-link class we set in constructor (https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/FormAutofillPreferences.jsm#89). If setting it after an element is attached to DOM, then it will solve this kind of problems.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #1)

I would guess this is a de-XBL regression from bug 1527495. Since this is the second regression from that bug with the same cause, maybe all the changes in that bug should be audited?

is it something QA can help with?

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 9046382 [details] [diff] [review]
patch

I can't get Splinter to load at the moment. Please try to use Phabricator since reviewing on BMO is going away soon.

>diff --git a/toolkit/content/widgets/text.js b/toolkit/content/widgets/text.js
>--- a/toolkit/content/widgets/text.js
>+++ b/toolkit/content/widgets/text.js
> 
>+  connectedCallback()
>+  {

Nit: put the opening brace on the same line as the method name

>+    this.classList.add("text-link");
>+  }
>+

Shouldn't we also do `super.connectedCallback()`?

(In reply to alexander :surkov (:asurkov) from comment #3)

> (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #1)
> 
> > I would guess this is a de-XBL regression from bug 1527495. Since this is the second regression from that bug with the same cause, maybe all the changes in that bug should be audited?
> 
> is it something QA can help with?

Since you're doing a more general fix I think this isn't as much necessary. If you were just going to fix the Form Autofill prefs then I think we would need to make sure we didn't need the same fix other places.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #4)

Shouldn't we also do super.connectedCallback()?

There's not currently a connectedCallback implemented on the base Custom Element class.

Comment 6

[Matthew N. [:MattN]]

(In reply to Brian Grinstead [:bgrins] from comment #5)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #4)

Shouldn't we also do super.connectedCallback()?

There's not currently a connectedCallback implemented on the base Custom Element class.

But that could change at any time, right? Why not make the code future-proof now?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #4)

Shouldn't we also do super.connectedCallback()?

There's not currently a connectedCallback implemented on the base Custom Element class.

But that could change at any time, right? Why not make the code future-proof now?

Wouldn't it throw if we added it here? super.connectedCallback would be undefined.

Comment 8

[Matthew N. [:MattN]]

(In reply to Brian Grinstead [:bgrins] from comment #7)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #4)

Shouldn't we also do super.connectedCallback()?

There's not currently a connectedCallback implemented on the base Custom Element class.

But that could change at any time, right? Why not make the code future-proof now?

Wouldn't it throw if we added it here? super.connectedCallback would be undefined.

Oh yeah, in Web Payments code we would check if it was truthy:

if (super.connectedCallback) {
  super.connectedCallback()
}

I wish it was defined on HTMLElement so you wouldn't have to check that.

Comment 9

[Brian Grinstead [:bgrins]]

If we wanted to enforce calls to super it would probably make sense to implement an empty function in the base class to avoid that check. I'm not sure I see a benefit to doing that eagerly instead of waiting until we need it and then updating consumers at that time (if we want to do this now we'll have to go through that same process anyway).

Relatedly, there's a bit of a complication here around delayConnectedCallback (should the call to super happen before or after that?), which can go away once XBL is removed from the frontend.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41885fe2a293
Autofill addresses learn more link is not blue, r=MattN

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/41885fe2a293