Closed Bug 1530211 Opened 7 months ago Closed 7 months ago

Autofill addresses learn more link is not blue

Categories

(Toolkit :: XUL Widgets, defect, P2, minor)

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- unaffected
firefox66 --- unaffected
firefox67 --- fixed

People

(Reporter: asa, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

Attached image image.png

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

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

Blocks: 1527495
Has Regression Range: --- → no
Flags: needinfo?(surkov.alexander)
Keywords: regression
OS: Windows 10 → All
Priority: -- → P2
Hardware: x86 → All
Attached patch patchSplinter Review

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.

Assignee: nobody → surkov.alexander
Flags: needinfo?(surkov.alexander)
Attachment #9046382 - Flags: review?(MattN+bmo)

(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 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.
Attachment #9046382 - Flags: review?(MattN+bmo) → review+

(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.

(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?

(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.

(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.

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.

Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41885fe2a293
Autofill addresses learn more link is not blue, r=MattN
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Component: Form Autofill → XUL Widgets
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.