1772376 - When inspecting FilterEditor.xhtml all search terms and actions are doubled.

Status: RESOLVED FIXED

(Thunderbird :: Filters, defect)

Description

[Axel Grude]

When inspecting FilterEditor.xhtml, all search terms and actions are doubled.

I believe this is caused by moving the processing of filter detail list from load to the DOMContentLoaded event. This also destroys all edit fields of custom search terms and custom actions added via the nsIMsgFilterService.addCustomAction() and nsIMsgFilterService.addCustomTerm() functions

Steps to reproduce:

1 - open filter list
2 - select any existing filter and edit ot
3 - open inspector (Ctrl+Shift+I)
4 - from the document selector (top right), select FilterEditor.xhtml

this will instantly duplicate all items in the searchterms and actions richtlist boxes.

I am currently working on a patch to re-add the broken action values that are missing from my FiltaQuilla Add-on. If there is no technical reason for moving from the load event to DOMContentLoaded in the new version I would propose trying to move the initialization back to the load event instead - if it fixes this issue it will also fix issues on my side too.

Comment 1

[Alessandro Castellani [:aleca]]

Thanks for reporting this.
I noticed happening it a couple of times, only when the dev tools is triggered.

IIRC, Henry worked on those filters to convert them in JS.
Gently pinging them to see if they can reproduce and fix it.

Comment 2

[Axel Grude]

Great - related bug on my github: https://github.com/RealRaven2000/FiltaQuilla/issues/164

I am halfway through a workaround which adds search terms via el.replaceWith(el) on <search-value> elements. Unfortunately this doesn't work with actions as they are added "empty" - with no child elements present. All previous patching code (which worked in Tb91) was executed before DOMContentReady and thus it was possible to let Thunderbird Core render all elements correctly.

Comment 3

[Magnus Melin [:mkmelin]]

This is from https://hg.mozilla.org/comm-central/rev/197babe349e1c9d713b6c6a34f9f6892c09eb866 - bug 1703164
Apparently inspecting the document will trigger another DOMContentLoaded event, which onload would not.

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to Axel Grude from comment #0)

When inspecting FilterEditor.xhtml, all search terms and actions are doubled.
This also destroys all edit fields of custom search terms and custom actions added via the nsIMsgFilterService.addCustomAction() and nsIMsgFilterService.addCustomTerm() functions

Not sure what this part is about. Do you mean due to the doubling?

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: Bug 1772376 - Prevent double loading of filter search terms/actions when inspecting the FilterEditor dialog. r=henry

Comment 6

[Axel Grude]

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

(In reply to Axel Grude from comment #0)

When inspecting FilterEditor.xhtml, all search terms and actions are doubled.
This also destroys all edit fields of custom search terms and custom actions added via the nsIMsgFilterService.addCustomAction() and nsIMsgFilterService.addCustomTerm() functions

Not sure what this part is about. Do you mean due to the doubling?

No there was some refactoring (in Tb91 the initialisation code ran in the onload event, now it runs on DOMContent loaded). I have 2 Add-ons (quickFilters and FiltaQuilla) that are using custom actions and custom terms and need to define their own custom search term / custom action edit elements in order to be edited, before Thunderbird populates the filter. Since the refactoring effort populates the filter earlier, none of my custom actions show edit fields - making both custom actions and custom search terms a thing of the past:

Expected result (from tb91.10):
https://i.imgur.com/SDJUOKU.png

Result in Tb102:
https://i.imgur.com/tkXAEcf.png

I have started writing a patch that restores the search terms (at least they contain some empty xul elements I can fill by reading the value from the filter and writing it back, then replacing the element with itself) - but the actions are completely empty, so I have currently no workaround. Not sure why the load event wasn't kept during refactoring, it definitely breaks anything custom... If there is a technical reason for using DOMContentLoaded I won't argue as I know Add-ons should not impose a "development tax". However it makes the existence of Custom Actions altogether a little questionable. When you add custom actions in the editor manually everything works as expected, it's just loading an existing filter that was broken.

My guess is that switching the code back to run on the onload event instead of DOMContentLoaded may actually fix both issues.

Comment 7

[Axel Grude]

Attachment: TB102 - Custom Filter Rules

Thunderbird 102.0b1:
The filter editor doesn't render any inputs for custom search terms and custom actions.

Comment 8

[Axel Grude]

Attachment: Thunderbird 91 .10 filter rules

The same filter, loaded in Thunderbird 91. For testing, the custom actions / search terms need to be enabled in FiltaQuilla.

Comment 9

[Axel Grude]

Attachment: test filter to import

If you want to test this behavior, here is a test filter relating to the screenshots. It can be imported with the restore filters button from quickFilters:
1 - install quickFilters Add-on
2 - open Message Filters
3 - click on the orange toolbarbutton "Restore Filters..."
4 - open the json file - this will import the test filter.

To see the missing custom items, enable them:
1 - Install the FiltaQuilla Add-on
2 - Open FiltaQuilla options
3 - On the "Actions" tab, enable Prepend to Subject, Remove Tag, Append to Subject, Launch File, Run Program, Detach Attachments To, Save Message as File, MOve Later, Add Sender to ADdress List, Javascript action, Play Sound, Forward with SmartTemplate, Reply with SmartTemplate
4 - On the Search Terms tab, enable Subject Regex Match and Bcc.

All these are custom actions / search terms that have editable fields which are missing in Tb102 when the editor loads. Related bug: https://github.com/RealRaven2000/FiltaQuilla/issues/164
I have attached a test version of FQ (3.5pre6) in the latest comment.

Comment 10

[Magnus Melin [:mkmelin]]

Is your initialization set up to run on load? If so, can't you change it to run on DOMContentLoaded instead?

Comment 11

[Axel Grude]

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

Is your initialization set up to run on load? If so, can't you change it to run on DOMContentLoaded instead?

No I can't because I am using John Bieling's WindowListener library to load my script:

background script:

messenger.WindowListener.registerWindow("chrome://messenger/content/FilterEditor.xhtml", 
   "content/scripts/filtaquilla-filterEditor.js");

as soon as the script is loaded it loads the patch script - from filtaquilla-filterEditor.js:

Services.scriptloader.loadSubScript(
  "chrome://filtaquilla/content/fq_FilterEditor.js", window, "UTF-8");

that script patches the getChildNode() functions for all relevant <ruleactiontarget-wrapper> elements so that the correct elements are injected.

I have no control on when the WindowListener loads these scripts but I think it always does it during onLoadWindow. It seems the change in Thunderbird Core from onLoad to OnDOMContentLoaded is quite fundamental and could have more unknown side effects. I would be interested whether there was a technical reason to change to this event rather than using onLoad? If not, then patching would be quite a small effort (and possibly fix the inspector issue as well)

Comment 12

[Magnus Melin [:mkmelin]]

To quote MDN DOMContentLoaded doucmnetation: "It is a common mistake to use load where DOMContentLoaded would be more appropriate."
It doesn't make a huge difference, but waiting for load delays things a bit.

John, do you know about WindowListener loading? Can/should it start using DOMContentLoaded?

Comment 13

[John Bieling (:TbSync)]

The onLoadWindow callback from ExtensionSupport.registerWindowListener fires on load. I have not looked into changing that, as that will affect all consumers and I feared that will have an even greater impact. And even then, it is not guaranteed that the add-on is triggered before core code. Right?

Comment 14

[Magnus Melin [:mkmelin]]

It's just another event, that triggers earlier. What guarantees of triggering order would be the same as earlier I think, just based on another event?

Comment 15

[Axel Grude]

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

It's just another event, that triggers earlier. What guarantees of triggering order would be the same as earlier I think, just based on another event?

Not sure I understand the question, but is it possible that DOMContentLoaded is triggered multiple times in the lifetime of the message editor window? Could it be triggered by the Inspector?

According to google: "The DOMContentLoaded event fires when the initial HTML document has been completely loaded and parsed, without waiting for stylesheets, images, and subframes to finish loading."

That is different to the window load event, according to mdn:

"The load event is fired when the whole page has loaded, including all dependent resources such as stylesheets and images."

I do not know what the WindowLoader API does to assure that the injected code runs before other things within the window load event, but it definitely has a big impact on potentially many Add-ons that use the WindowLoader API because their functions are not yet available as regular APIs.

Comment 16

[Magnus Melin [:mkmelin]]

Maybe registerWindowListener should add another function for DOMContentLoaded events?
I guess for this bug we can use onload, but IIRC using DOMContentLoaded was in other cases needed to get the proper loading sequence, so it's not a general fix.

DOMContentLoaded shouldn't happen more than once. In this bug, it must be picking up DOMContentLoaded from wrong scope somehow.

Comment 17

[John Bieling (:TbSync)]

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

Maybe registerWindowListener should add another function for DOMContentLoaded events?
I guess for this bug we can use onload, but IIRC using DOMContentLoaded was in other cases needed to get the proper loading sequence, so it's not a general fix.

DOMContentLoaded shouldn't happen more than once. In this bug, it must be picking up DOMContentLoaded from wrong scope somehow.

That sounds like a good plan, we do not break existing add-ons but allow to use DOMContentLoaded. Will take care of that in another bug.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/58d2015d29d5
Prevent double loading of filter search terms/actions when inspecting the FilterEditor dialog. r=henry

Comment 19

[Magnus Melin [:mkmelin]]

I'll have to back out this and land with the "once" solution instead.
As it were, using DOMContentLoaded had a good reason. Without it, timings make it impossible to create the first filter in a new profile (try -safe-mode) - the dialog simply doesn't come up.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dd7d83a04985
Backed out changeset 58d2015d29d5 . rs=backout
https://hg.mozilla.org/comm-central/rev/fc9803c685af
Prevent double loading of filter search terms/actions when inspecting the FilterEditor dialog. r=henry

Comment 21

[Axel Grude]

(In reply to John Bieling (:TbSync) from comment #17)

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

Maybe registerWindowListener should add another function for DOMContentLoaded events?
I guess for this bug we can use onload, but IIRC using DOMContentLoaded was in other cases needed to get the proper loading sequence, so it's not a general fix.

DOMContentLoaded shouldn't happen more than once. In this bug, it must be picking up DOMContentLoaded from wrong scope somehow.

That sounds like a good plan, we do not break existing add-ons but allow to use DOMContentLoaded. Will take care of that in another bug.

John, could you make a patch for testing on the next Add-on meeting (Monday evening)? Do you intend to add a parameter for event type or a simple toggle switch? Hoping this is not going to have other side effects so I want to test this ASAP to hit the ground running for the next beta. Hopefully this earlier event doesn't open any race conditions. (also not sure how you make sure that our scripts are loaded first - is there an even earlier event?)

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9279828 [details]
Bug 1772376 - Prevent double loading of filter search terms/actions when inspecting the FilterEditor dialog. r=henry

[Approval Request Comment]
Regression caused by (bug #): bug 1703164
User impact if declined: problems using the inspector on this dialog
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): safe (note, the revision to land is https://hg.mozilla.org/comm-central/rev/fc9803c685af only)

Comment 23

[Wayne Mery (:wsmwk)]

Comment on attachment 9279828 [details]
Bug 1772376 - Prevent double loading of filter search terms/actions when inspecting the FilterEditor dialog. r=henry

[Triage Comment]
Approved for beta

Rob, Note, the revision to land is https://hg.mozilla.org/comm-central/rev/fc9803c685af only

Comment 24

[Rob Lemley [:rjl]]

Thunderbird 102.0b5:
https://hg.mozilla.org/releases/comm-beta/rev/967992f4a0cc

Comment 25

[John Bieling (:TbSync)]

(In reply to John Bieling (:TbSync) from comment #17)

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

Maybe registerWindowListener should add another function for DOMContentLoaded events?
I guess for this bug we can use onload, but IIRC using DOMContentLoaded was in other cases needed to get the proper loading sequence, so it's not a general fix.

DOMContentLoaded shouldn't happen more than once. In this bug, it must be picking up DOMContentLoaded from wrong scope somehow.

That sounds like a good plan, we do not break existing add-ons but allow to use DOMContentLoaded. Will take care of that in another bug.

We solved that differently, without patching the core module. Instead, we use a dedicated Experiment. Changing the core implementation was not easily possible without risking side effects.
FYI: https://github.com/RealRaven2000/FiltaQuilla/commit/4628e91896acef25e3ce0fe8076ddb24bf675f89

I added that info here for completeness, but we should not discuss anything related to that issue in this bug. This bug is fixed.