1652072 - No find highlight in sub dialogs

Status: NEW

(Thunderbird :: Preferences, defect)

Description

[Richard Marti (:Paenglab)]

When I search in the preferences for example for propo the search results shows me that this word exists in the Fonts sub dialog. When I open the dialog, the matching text isn't highlighted. In FX the text is highlighted in sub dialogs too.

Comment 1

[Thomas D. (:thomas8)]

Also, I have seen what seems to be false positives:
Search Options for "Update", there's a yellow bubble pointing to Set Alternatives button of Language section but the word "update" is nowhere to be seen in the Thunderbird language settings dialog that results from clicking the button.
Can we cover that problem here?

Comment 2

[Richard Marti (:Paenglab)]

(In reply to Thomas D. from comment #1)

Also, I have seen what seems to be false positives:
Search Options for "Update", there's a yellow bubble pointing to Set Alternatives button of Language section but the word "update" is nowhere to be seen in the Thunderbird language settings dialog that results from clicking the button.
Can we cover that problem here?

It finds the "update" in the error message https://searchfox.org/comm-central/rev/779acbf88f47004e588d3448f7ff4d2d46055737/mail/locales/en-US/messenger/preferences/languages.ftl#45

FX doesn't show it and has this message too.

Comment 3

[Alessandro Castellani [:aleca]]

This implementation was handled by Khushil, let's see if he has an idea about the issue.
Would you be able to check if this is a regression caused by a follow up bug or it's an issue coming directly from the original implementation of the search?

Comment 4

[Khushil Mistry [:khushil324]]

In Firefox, they have content as textNode in xul:label whereas we have content as "value" property in few of the labels in the dialogs so it's not possible to highlight those labels.

Comment 5

[Magnus Melin [:mkmelin]]

Can the function that does it be adjusted to account for value?

Comment 6

[Khushil Mistry [:khushil324]]

I am not sure about that.
We are doing it here: https://searchfox.org/comm-central/source/mail/components/preferences/findInPage.js#154,197,208
Any thoughts?

Comment 7

[Magnus Melin [:mkmelin]]

Could maybe special case <xul:label> to look at value too: https://searchfox.org/comm-central/rev/b9e582ccc705a1e24c8b46227e072bdfa00aeaf8/mail/components/preferences/findInPage.js#122 - but maybe more is required as well.

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to Magnus Melin [:mkmelin] from comment #7)

Could maybe special case <xul:label> to look at value too: https://searchfox.org/comm-central/rev/b9e582ccc705a1e24c8b46227e072bdfa00aeaf8/mail/components/preferences/findInPage.js#122 - but maybe more is required as well.

Is this better done in the post-xul world?

Comment 9

[Alessandro Castellani [:aleca]]

Mh, this shouldn't be affected by xul or html, as in FF is working with it.

This might be a case of fluent implementation as I see this difference:

• m-c: https://searchfox.org/mozilla-central/rev/ac7da6c7306d86e2f86a302ce1e170ad54b3c1fe/browser/locales/en-US/browser/preferences/fonts.ftl#78-79
• c-c: https://searchfox.org/comm-central/rev/11db1160f98ef139414286d8427c651bb5f658ee/mail/locales/en-US/messenger/preferences/fonts.ftl#22-24

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2021-09-01 15-20-58.png

Yup, confirmed.
Not using the .value in the fluent file fixes it.

Comment 11

[Alessandro Castellani [:aleca]]

Henry, would you able to handle this?
It's a bit of a tedious chore as it requires going through all the strings in the preferences and its subdialogs, and being sure they are properly highlighted when searching.
You can split the fixes in multiple patch if you want.

Comment 12

[Alessandro Castellani [:aleca]]

Let's hold on a bit before working on this as this can turn into a substantial round of string changes, and let's find a proper approach before coding.

I can think of 2 different approaches we might do:

1. Update all the necessary strings by removing the .value and consequentially changing the fluent IDs.
2. or, Update all the necessary strings by removing the .value, but we DON'T change the fluent IDs and instead we create dedicated migration recipes to guarantee that the translated strings are properly maintained.

Maybe solution 2 would be better for localizers since we're not actually changing strings, but it's extra work for us and I don't know when we might be able to run those migrations.

Magnus, what do you think?

Comment 13

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

Just to add to the previous discussion. Ideally, we would use html:labels. However, html:labels are stricter than xul:labels because their for attribute must point to a labelable element (https://html.spec.whatwg.org/multipage/forms.html#category-label), unlike the control attribute. I tried to use a html:label with a xul:menulist and it didn't work. So xul:labels should probably be converted to html at the same time as their corresponding form elements, if they are currently xul elements.

Another thing to point out is that we already have a custom highlightable-button element that was created exactly so the contained xul:label would use a Text node, rather than the value attribute https://searchfox.org/comm-central/rev/88d88aa4086176def5c63925b9d727b593ec6b99/mail/components/preferences/findInPage.js#14-20 . I wonder if an easy fix, whilst we still use xul, would be a similar highlightable-label element that remaps the value attribute to its textContent?

However, when we convert to html, we will need to remap the .value to the text content anyway, so we might as well do it now for the xul:labels.

I wouldn't mind going through and searching for places that use the value attribute instead of text content.

Comment 14

[Magnus Melin [:mkmelin]]

Not to conflate issues, it would seem comment 7 is a fix. Converting to html is the topic for other bugs. It's certainly possible some variation of aleca's suggestion would be needed in the future, but it could be better to see what the structure in markup is first.

Comment 15

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

(In reply to Magnus Melin [:mkmelin] from comment #14)

Not to conflate issues, it would seem comment 7 is a fix. Converting to html is the topic for other bugs. It's certainly possible some variation of aleca's suggestion would be needed in the future, but it could be better to see what the structure in markup is first.

I'm not sure this would work. The current approach uses Selection for highlighting, I think this is incompatible with <label value=>. For example,

label = document.createXULElement("label");
label.value = "test";
range = document.createRange();
range.setStart(label, 2);
// DOMException: Index or size is negative or greater than the allowed amount
// Since this is an element node, the only allowed index would be `0`, we cannot select part of the label.

// Compared to
label.removeAttribute("value");
label.textContent = "test";
range.setStart(label.firstChild, 2);

Comment 16

[Magnus Melin [:mkmelin]]

I didn't have a look where the highlighting is done. The code could easily put the label.value into label.textContent though so that the selection would work like you describe.

Comment 17

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

Ah, that could work: transforming the xul:label in the highlight method itself, as needed. Some thoughts:

1. It is a little bit of a hack, and it would be unusual behaviour for a highlighting method to change the structure of an element. Someone working in the markup might not anticipate this.
2. It could cause intermittent problems for methods that read the label value. Note that changing the markup would also cause the same problem, but it would be easier to spot because it would not depend on whether the user performed a search.
3. Labels that change the text they display would need to take care to take into account that the current text could either be in the value attribute, or the textContent. However, I'm not sure we have many such labels in preferences.
4. We might also have to consider labels that are internal to other xul elements (like buttons and menu items), which might also be dynamic.

We could take a safer approach and only enable it for xul:label elements that are marked with the class "highlightable-xul-label" (the "xul" part should hopefully remind people that they can remove the class when converting to HTML), to make the behaviour opt-in and to flag it in the markup itself. This would address points 1, 3 and 4 above.