1842547 - Search Shortcuts settings missing headers / header labels don't update dynamically

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Blake]

Steps to reproduce:

Go to about:preferences#search --> "Search Shortcuts" section

Actual results:

Table is missing headers

Expected results:

Table should have headers to identify columns

Comment 1

[Blake]

mozregression points to Bug 1831259. First bad build 2023-06-08.

build_date: 2023-06-07 23:32:48.148000
build_type: integration
build_url: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/bbT3afTcR8eNNpbArQLlvA/runs/0/artifacts/public%2Fbuild%2Ftarget.zip
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=20056e07ec0b6b8f880493d89974e85d192775fa&tochange=f0bb54e03054060649af1c6d5025c09c4e85ebf2
repo_name: autoland
task_id: bbT3afTcR8eNNpbArQLlvA

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::Settings UI' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

:Gijs, since you are the author of the regressor, bug 1831259, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[:Gijs (out for now; he/him)]

Eemeli, can you help me understand why these elements are not being translated by fluent here?

They end up in an open shadowroot. Before the regressing change, we explicitly called translateFragment before the DOM fragment was inserted into the "real" DOM, which meant that the treecol elements like #engineKeyword were not in any shadowroot (ie "just" light DOM).

I know that for closed roots, the root needs to add itself to fluent's collection of roots. I didn't think that was the case for open shadowroots?

(and more generally, do we still stand any chance of changing this design aspect of fluent? It's kind of tedious that anyone who implements a custom element has to remember to do fluent admin in case its consumers need it to work...)

Comment 5

[:Gijs (out for now; he/him)]

[Tracking Requested - why for this release]:
Seems bad not to have column labels here.

Comment 6

[:Gijs (out for now; he/him)]

... actually, it seems the label attribute is being set, but it's not correctly inheriting down into the treecol's content, the way this code suggests that it should...

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1831259

Comment 8

[:Gijs (out for now; he/him)]

Argh, of course, it's something dumb...

bug 1820634 added an attributeChangedCallback to tree.js's treeCol element but it doesn't call super's attributeChangedCallback, so it broke the `inheritedAttributes functionality provided by the superclass.

Comment 9

[:Gijs (out for now; he/him)]

Attachment: Bug 1842547 - ensure treecol label, crop and other attributes actually inherit correctly, r?emilio

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b9d8da8776f
ensure treecol label, crop and other attributes actually inherit correctly, r=emilio

Comment 11

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/1b9d8da8776f

Comment 12

[:Gijs (out for now; he/him)]

Attachment: Bug 1842547 - ensure treecol label, crop and other attributes actually inherit correctly, r?emilio

Original Revision: https://phabricator.services.mozilla.com/D183207

Comment 13

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: No
• Fix verified in Nightly: no
• Steps to reproduce for manual QE testing: See comment 0
• Explanation of risk level: This is reinstating a call that used to exist prior to bug 1820634 (landed in 112) and shouldn't ultimately affect many <treecol> consumers anyway
• Code covered by automated testing: no
• Is Android affected?: no
• Needs manual QE test: yes
• User impact if declined: Missing labels in the UI
• Risk associated with taking this patch: minimal

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Gijs, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox116 to wontfix.

For more information, please visit BugBot documentation.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 16

[:Gijs (out for now; he/him)]

I did request uplift... Filed https://github.com/mozilla/bugbot/issues/2167 to fix the bot.

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/4444bd2d76ad

Comment 18

[Dianna Smith [:diannaS]]

Comment on attachment 9343372 [details]
Bug 1842547 - ensure treecol label, crop and other attributes actually inherit correctly, r?emilio

Approved for 116.0b5

Comment 19

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-07-09 Nightly build on Windows 10.
Verified as fixed on Firefox 116.0b5(build ID: 20230713175905) and Nightly 117.0a1(build ID: 20230713214846) on Windows 10, macOS 12, Ubuntu 22.
Search Shortcut table now displays the header properly.

Comment 20

[Henry Wilkes (they/them) [:henry-x]]

esr115 is also effected.

Can we get an uplift for esr115?

Comment 21

[:Gijs (out for now; he/him)]

(In reply to Henry Wilkes (they/them) [:henry-x] from comment #20)

esr115 is also effected.

Can we get an uplift for esr115?

bug 1831259 didn't land there, so I don't think this bug should be reproducible on Firefox esr115 (hence that release being marked as unaffected). Can you clarify how you're reproducing an issue there?

Comment 22

[Henry Wilkes (they/them) [:henry-x]]

We have the same problem with missing labels in xul:treecol in tor browser. Basically, setting label attribute on the treecol will not update the xul:label element's value as per the inheritedAttributes.

I think the regression was from https://hg.mozilla.org/integration/autoland/rev/57b5df18c27c#l7.12. I.e. bug 1820634 which landed in 112.

Comment 23

[Henry Wilkes (they/them) [:henry-x]]

I guess it is only really an issue if you set the label after connectedCallback is called. I'm not sure if this happens within firefox though.

Comment 24

[Henry Wilkes (they/them) [:henry-x]]

(In reply to Henry Wilkes (they/them) [:henry-x] from comment #23)

I guess it is only really an issue if you set the label after connectedCallback is called. I'm not sure if this happens within firefox though.

Actually, this does happen when you switch locales. So in firefox 115.2.1esr, if I switch from "en-US" to another language, the existing tree columns will still have their en-US titles despite their "label" attribute being changed to the new language.

Comment 25

[:Gijs (out for now; he/him)]

(In reply to Henry Wilkes (they/them) [:henry-x] from comment #22)

We have the same problem with missing labels in xul:treecol in tor browser. Basically, setting label attribute on the treecol will not update the xul:label element's value as per the inheritedAttributes.

Does tor browser do this in places where Firefox does not? Can you clarify where this is? :-)

(In reply to Henry Wilkes (they/them) [:henry-x] from comment #24)

(In reply to Henry Wilkes (they/them) [:henry-x] from comment #23)

I guess it is only really an issue if you set the label after connectedCallback is called. I'm not sure if this happens within firefox though.

Actually, this does happen when you switch locales. So in firefox 115.2.1esr, if I switch from "en-US" to another language, the existing tree columns will still have their en-US titles despite their "label" attribute being changed to the new language.

Thanks, this is helpful. There are many other places where the live switching does not work perfectly so I think this alone would not justify ESR uplift... also because it'd be fixed as soon as you refreshed the settings/prefs. But I guess at least it allows QA to verify the fix on ESR.

Comment 26

[:Gijs (out for now; he/him)]

Attachment: Bug 1842547 - ensure treecol label, crop and other attributes actually inherit correctly, r?emilio

Original Revision: https://phabricator.services.mozilla.com/D183207

Comment 27

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: No
• Code covered by automated testing: no
• Risk associated with taking this patch: Low
• Is Android affected?: no
• Steps to reproduce for manual QE testing: N/A
• User impact if declined: ESR / tor browser brokenness when column header labels are set dynamically
• Fix verified in Nightly: yes
• Explanation of risk level: Minor changes that ensure a subclass calls superclass's "update" methods
• Needs manual QE test: no

Comment 28

[Henry Wilkes (they/them) [:henry-x]]

Does tor browser do this in places where Firefox does not? Can you clarify where this is? :-)

Sure, it is in our own saved onion keys dialog (Privacy & Security, Onion Services Authentication, Saved Keys...): https://gitlab.torproject.org/tpo/applications/tor-browser/-/issues/42109

We've not yet migrated our old strings to fluent yet, so the table's strings are assigned just after the page load.

Comment 29

[Pascal Chevrel:pascalc]

Comment on attachment 9353660 [details]
Bug 1842547 - ensure treecol label, crop and other attributes actually inherit correctly, r?emilio

Approved for ESR 115.3, thanks.

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/17bedcda2f5c

Comment 31

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-07-09 Nightly build on macOS 12.
Verified as fixed on Firefox ESR 115.3.0(build ID: 20230918121054) on macOS 12, Windows 10, Ubuntu 22.