Fixed Brief Description Popover extraction Issue

[THEBOSS0369]

This PR Fixes #1276

Hello Everyone!

Description

In this PR i have fixed the popover inappropriate description extraction by adding regex logic so that only Required information will be shown.
NOTE: This PR fixes almost every popover however there are still 3 - 5 popover's that i haven't able to fix for example calsacaus in Europe , Greenland and other 3 from USA.

Test

I have done all the necessary test

1. Checked in both "Restricted" and "ServiceWorker" Everything is working fine check the ss below.
2. Unit tests npm test no issue
3. End-to-end (e2e) tests-e2e-iemode-> There was some e2e.runner.js error which i have commented on this PR's Issue thread. I tried alternative http-server there wasn't any issue.
4. extension versions with production code tested
5. Browser Test -> Microsoft Edge, Chrome, Firefox and Brave

Screenshots

Before

After (Test - eServiceworker / Non restricted)

After (Test - Restricted)

Thanks

[Jaifroid]

Your issue is probably due to the lack of a standard Firefox installation locally. I'm running tests on GH Actions, so let's see.

[Jaifroid]

Hi, all tests passed, and I've also now had a chance to test this on the Wikivoyage that was showing it. It's working fine, well done! I just need to check for regressions with any other Wikimedia archives, which I'll do now.

[Jaifroid]

Please see comment on why we can't use length as a selection criterion. However, I saw while testing that these title descriptions do in fact have an ID which can be used to filter them out. See screenshots:

All you need to do, then, is refine the querySelectorAll function on line 107 so that it selects all paragraphs EXCEPT those with the id #pcs-edit-section-title-description.

[Jaifroid]

I don't think this is a safe way to filter out these short descriptions. We don't know in advance the amount of text, so setting a limit of 100 characters seems arbitrary and will likely have unintended consequences elsewhere, for example filtering out legitimate paragraphs with only 100 characters in them.

[Jaifroid]

The reason we do have a filter requiring at least 50 characters is that there are lots of small empty paragraphs or empty paragraphs with things like <span>&nbsp;</span> which can't be filtered out any other way. Through testing, we determined that 50 characters caught nearly all these while never removing any nodes with meaningful text.

[THEBOSS0369]

Got it Sir! i will change the logic.

[Jaifroid]

Good idea to update the branch too, see button "Update branch" below. Once you've done that, be sure to pull the change in your local copy of the Repo.

[THEBOSS0369]

Hey @Jaifroid !

Apologies for late reply, i wasn't able to open github as i had exams which ended today.

So for this PR i was also not very confident because i was directly changing the hex code, that's why i created it thanks for identifying this.
Also can you tell me how did you find the Description's id in inspect mode like if i have to find any id i used to search the whole components ID (i know its childish) but i never thought that we can do this with console as well😅😅.

Also in summary now i have to make the changes so that the extraction will be done from the ID , right?

Thanks!

[Jaifroid]

@THEBOSS0369 I simply put a break on line 109 of popover.js using the browser's dev tools, and then when the execution paused, I inspected the value of the node variable. The first paragraph of the collection clearly contains the ID #pcs-edit-section-title-description. Hope that helps!

[THEBOSS0369]

@Jaifroid ! Thanks for this info sir, it really helped!! :)

[THEBOSS0369]

Hey @Jaifroid !

I have fixed the requested changes. Please review it. This time every popover is working fine.

This the ss to verify the #pcs-edit-section-title-description is been excluded in the popover.

and these are some ss of popover which weren't working before but now working.

Thanks!

[Jaifroid]

This is a great improvement, many thanks!

Apart from the specific comments, here's how you could generalize the :not selector:

At the beginning of the cleanupLedeContent function, add the variables defining exclusion styles, e.g.:

// Define an array of exclusion filters (note .exclude-this-class is a dummy class as an example)
const exclusionFilters = ['#pcs-edit-section-title-description', '.exclude-this-class'];
// Construct the :not() selector string
const notSelector = exclusionFilters.map(filter => `:not(${filter})`).join('');

Then change your querySelectorAll definition to use this, e.g. p${notSelector}. Also add a comment before this, to explain what we're doing. Something like "Apply the style-based exclusion filters to remove unwanted paragraphs".

This uses the same solution you came up with, but would make it more flexible for future developers who need to add more exclusion filters.

[THEBOSS0369]

@Jaifroid ! I have fixed all the changes you asked me. Everything is working fine after these changes. Here is another ss for the proof which was in the issue.

Thanks !

[Jaifroid]

This is looking good. Just a few typos to correct! I've done these as suggestions, so you can simply accept them if you wish.

[THEBOSS0369]

This is looking good. Just a few typos to correct! I've done these as suggestions, so you can simply accept them if you wish.

@Jaifroid These suggestions are definitely great i have commited those changes! Also Is there anything else needed to be done for this issue or The issue is fixed Now.

[Jaifroid]

@THEBOSS0369 Thank you very much for this PR. It looks great now! I'll do some final manual testing before merging it. Are you OK for me to merge it after testing if all is good? The PR will be in your name and credited to you.

[THEBOSS0369]

@THEBOSS0369 Thank you very much for this PR. It looks great now! I'll do some final manual testing before merging it. Are you OK for me to merge it after testing if all is good? The PR will be in your name and credited to you.

@Jaifroid Yes Sir! No Problem at all!