(Updated) Fixes Multiple TODO's in Code #4678

Govi2020 · 2025-04-10T18:59:45Z

Description

All changes made were made according to TODO's that was in the comments in the code.

pages/options.js --> Now if there is a error then the window will scroll into view to show the error and input in validation
content_scripts/mode_visual --> has unneeded closure, which was changed into plain for loop
content_scripts/linkhints.js --> css was hard coded for popups which was changed just adding a classname
content_scripts/vimium.css -> moved the hard coded css to this file as recommend by the dev

I made originally made changes in Pull Request where due to formatting issues it created a lot of garbage changes which made it difficult to understand the real changes.

I thought it was better to discard all of that changes and edit from the latest version of vimium. So i have only made some small of incredibly simple changes to the code. This is my first time contributing to a open source project and i intent to make some more changes for this project. Thank you :)

philg-dev · 2025-04-10T20:08:36Z

content_scripts/link_hints.js

@@ -424,23 +424,11 @@ class LinkHintsMode {
      this.containerEl.appendChild(el);
    }

-    // TODO(philc): 2024-03-27 Remove this hasPopoverSupport check once Firefox has popover support.


This TODO comment has just been removed without taking the proper action that it mentions.

Firefox supports the Popover API since January 2025, i.e. Version 125 as can be seen here:
https://developer.mozilla.org/en-US/docs/Web/API/Popover_API#browser_compatibility

Therefore the hasPopoverSupport flag can be removed from this piece of code and the if condition removed (obviously while preserving the statements inside the if-block).

Furthermore I think this will also have an impact on the required minimum Version of Firefox that Vimium supports. This is mentioned in the CONTRIBUTING.md section Coding Style.

and needs to be changed in the manifest.json as far as I know. From what I've seen the minimum Version for Chromium browsers is also outdated for this piece of functionality.

But that means people with outdated versions cannot use it.
According to Caniuse (https://caniuse.com/mdn-api_htmlelement_popover) 90.15% seems to have popover support. and for tracked desktop users it shows 88.2%.
I checked the default version of firefox which comes with kali , and it checks out too.

should we perform the change. or should we write code to not use popover if it does not supports it and write our custom popover element?
what you think is the best decision, i think both are great options considering 90.15% support of popover overall.I will start working on it after your signal.

Well, I'm not the one to make that decision. I'm just another recent contributor and I've had an eye on most things that have been going on with the repo in the past couple of months.

I guess my main point was: either you don't remove the flag & check and leave the TODO just as-is, because nothing has been done about that part of the TODO or you follow the instructions of the TODO and remove it. In your change, you just removed the TODO comment and didn't change the related code.

In this case it's probably best to ask @philc for his opinion. Judging from the TODO comment, I would assume he's fine with removing the check. I'm not so sure what he thinks about the changes in minimum supported browser versions though.

I'd probably leave the TODO for a few more months until Firefox 125+ adoption creeps up higher, and then remove it.

@Govi2020 as philc decided, we should leave the comment about hasPopoverSupport in the code for now. The comment is still missing on your latest changes(?)

I have added it

content_scripts/link_hints.js

content_scripts/vimium.css

pages/options.js

philg-dev

Overall the PR consists of a bunch of changes that are otherwise unrelated to each other. As you can see from the comments, even some tiny changes can sometimes lead to further discussions / considerations... So in the future it might be better to split your PRs into self-contained units that semantically belong together.

See #4673 (comment)

Second, probably at least some of these should be broken out into their own PRs so they're easier to review and merge, rather than bundled together.

philg-dev · 2025-04-11T13:05:50Z

pages/options.js

@@ -243,12 +243,34 @@ const OptionsPage = {
    return result;
  },

+  isElementInView(element) {


Hmm... I've seen this or very similar functionality used in different spots around the project. Maybe this would be a good chance to centralize it?

@philc do you think this function would be better suited to be put into dom_utils.js?

I've seen similar but not identical usages in:

scroller.js#checkVisibility

mode_find.js

mode_visual.js

visibility_test.html

It might be worth considering consolidating at least some of those implementations.

@philc what do u think is the right move here

pages/options.js

philg-dev

As per my comments on some of the changes, I'm leaving this code review as a partial review. I've only gone over these files and then stopped for reasons mentioned in my comment on content_scripts/mode_visual.js:L334.

background_scripts/exclusions.js
content_scripts/link_hints.js
content_scripts/mode_visual.js

I did not review the changes in the remaining files.

philg-dev · 2025-04-14T19:04:03Z

content_scripts/link_hints.js

@@ -424,23 +424,11 @@ class LinkHintsMode {
      this.containerEl.appendChild(el);
    }

-    // TODO(philc): 2024-03-27 Remove this hasPopoverSupport check once Firefox has popover support.


@Govi2020 as philc decided, we should leave the comment about hasPopoverSupport in the code for now. The comment is still missing on your latest changes(?)

philg-dev · 2025-04-14T19:19:40Z

content_scripts/mode_visual.js

-        );
-        if ((selectionRect.height >= 0) && (selectionRect.width >= 0)) {
+
+        if (DomUtils.isElementVisible(this.selection.getRangeAt(0), { partialCheck: true })) {


What in the world is this change?!

Why would you completely remove the section that was implemented for the DOM Tests?

Also as per my recent comment regarding the centralization of the isElementVisible function: That was still up for debate and waiting for input from the maintainers. Instead it looks like you yourself just marked the comment as "resolved".

I'm not trying to be off-putting or rude, but I'm not putting in the time to review this PR every 5 minutes, when it contains all sorts of stuff that is still being discussed, with no decision made yet. Fairly "trivial" changes like this might be quick to implement, but the time it takes for reviewers to make sense of them - especially when nothing was properly discussed / decided upon yet - is often much more than the time it takes to implement.

Yea, you are right i was just too fast and excited to jump in and make change. I'll revert the changes for now and let it stay as it is when i can code.
I am very sorry for the inconvenience as i said i am new to contributing to open projects. I mostly worked on personal projects or work that i maintained. It was my mistake, sorry again

Govi2020 added 3 commits April 10, 2025 23:05

Now if there is an vaidation error it will scroll to the error

10f1a8e

Removed Comment, Moved static CSS from js to vimium.css

1aed670

Removed Unneeded Closure

b429efd

Govi2020 mentioned this pull request Apr 10, 2025

Fixes Multiple TODO's in Code #4673

Closed

Govi2020 changed the title ~~Fixes Multiple TODO's in Code~~ (Updated) Fixes Multiple TODO's in Code Apr 10, 2025

philg-dev suggested changes Apr 10, 2025

View reviewed changes

Govi2020 requested a review from philg-dev April 11, 2025 08:15

Reworked Scroll into view (options page) and reformatted css

7275d35

philg-dev reviewed Apr 11, 2025

View reviewed changes

Govi2020 requested a review from philg-dev April 14, 2025 04:40

philg-dev suggested changes Apr 14, 2025

View reviewed changes

Govi2020 force-pushed the master branch from c4f25be to 7275d35 Compare April 15, 2025 07:43

Added TODO in link_hints

20f63d1

Govi2020 requested a review from philg-dev April 16, 2025 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Updated) Fixes Multiple TODO's in Code #4678

(Updated) Fixes Multiple TODO's in Code #4678

Govi2020 commented Apr 10, 2025

philg-dev Apr 10, 2025

Govi2020 Apr 11, 2025 •

edited

Loading

philg-dev Apr 11, 2025

philc Apr 13, 2025

philg-dev Apr 14, 2025 •

edited

Loading

Govi2020 Apr 15, 2025

philg-dev left a comment

philg-dev Apr 11, 2025

Govi2020 Apr 15, 2025

philg-dev left a comment •

edited

Loading

philg-dev Apr 14, 2025 •

edited

Loading

philg-dev Apr 14, 2025

Govi2020 Apr 14, 2025 •

edited

Loading

(Updated) Fixes Multiple TODO's in Code #4678

Are you sure you want to change the base?

(Updated) Fixes Multiple TODO's in Code #4678

Conversation

Govi2020 commented Apr 10, 2025

Description

philg-dev Apr 10, 2025

Choose a reason for hiding this comment

Govi2020 Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

philg-dev Apr 11, 2025

Choose a reason for hiding this comment

philc Apr 13, 2025

Choose a reason for hiding this comment

philg-dev Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Govi2020 Apr 15, 2025

Choose a reason for hiding this comment

philg-dev left a comment

Choose a reason for hiding this comment

philg-dev Apr 11, 2025

Choose a reason for hiding this comment

Govi2020 Apr 15, 2025

Choose a reason for hiding this comment

philg-dev left a comment • edited Loading

Choose a reason for hiding this comment

philg-dev Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

philg-dev Apr 14, 2025

Choose a reason for hiding this comment

Govi2020 Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Govi2020 Apr 11, 2025 •

edited

Loading

philg-dev Apr 14, 2025 •

edited

Loading

philg-dev left a comment •

edited

Loading

philg-dev Apr 14, 2025 •

edited

Loading

Govi2020 Apr 14, 2025 •

edited

Loading