Change icon: overlaid button to background image #462

Vinnl · 2023-01-27T10:48:09Z

This PR fixes #22, #80, #227, #253, #360, #358, #394, #397, #400, #429, #447, #454, #497, #509.

(For anyone coming across this PR: we'll still need to do some extensive testing to make sure this doesn't break commonly-used websites, so please be patient — it may still take a while to land.)

New feature description

This applies a new method of injecting the Relay icon that I prototyped a while ago; a helpful description of this method can be found here, by @czlucius who independently landed on the same approach.

With this method, we don't have to change the stacking order of the website, and thus should avoid a lot of issues where we break the website we're injecting in. It did require some extra work to stay accessible; we still inject a button that is visually hidden, but accessible to screen reader users and people who navigate using the keyboard. I've also modified the hover/focus state styling to make it easier. Scrolling with the menu open is no longer possible; I can probably re-enable that with a re-positioning scroll listener, but I feel like the user experience is probably fine if you dismiss the menu first, and should be better for performance.

Note that I carried over the code in src/js/other-websites/icon.js from the experimental extension I prototyped this in, so the code style might be a little different from the rest of the codebase - the experimental extension had TypeScript and helpers I've to rip out.

Screenshot (if applicable)

Screencast.from.2023-01-27.11-26-42.webm

How to test

Visit websites with the Relay icon, and make sure that everything looks and works OK.

Checklist

l10n changes have been submitted to the l10n repository, if any.
All acceptance criteria are met.
All UI revisions follow the coding standards, and use Protocol tokens where applicable (see /frontend/src/styles/tokens.scss).
Commits in this PR are minimal and have descriptive commit messages.

Vinnl · 2023-01-27T11:59:11Z

src/js/other-websites/icon.js

+      // When generating a new mask, `newRelayAddressResponse` contains a mask object
+      // with the `full_address` property. When selecting an existing mask,
+      // however, it only has the properties `address` and `currentDomain`, but
+      // `address` does contain the full address (i.e. including @mozmail.com):


I miss TypeScript :(

Vinnl · 2023-01-31T13:45:17Z

Added two more features based on PM feedback:

Scrolling is no longer disabled, and the menu moves along with the icon on scroll: eb2c59c
If there's not enough space for the menu below the icon, and there is enough space on top, move the menu to the top: e5cfe23

Edit: also fixed some issues with metrics events being sent twice.

maxxcrawford

This PR has been a long time coming! Thanks, @Vinnl (and @czlucius too!) We'll need to do some extensive testing across sites to confirm there's no major breakage, but this so less invasive from our current approach.

Approving with some nits/suggestions/questions!

One minor UI inconsistency to call-out:
The in-page menu has both rounded corners and a small tooltip pointing towards the input:

I called out in the code where I believe it would be set, if that helps?

maxxcrawford · 2023-02-02T16:49:34Z

src/js/other-websites/email_detector.js

+  const detectedInputs = email_detector_ruleset
+    .against(domRoot)
+    .get("email")
+    .filter((node) => node.scoreFor("email") > 0.5)
+    .map((node) => node.element);
+
+  return typeEmailInputs.concat(detectedInputs);


Question: Why this re-write?

It was a generator function, where that didn't add much value as far as I could see (checking the commit log), while making it hard to pass around the list of email inputs. And then the rest was to make sure I actually understood what was going on here, and writing it myself was a good way to do so :) But happy to change it back to for loops if you prefer, now that I know what's going on :)

src/js/other-websites/fill_relay_address.js

src/js/other-websites/icon.js

maxxcrawford · 2023-02-02T17:28:31Z

src/js/other-websites/icon.js

+function intersectsRelayIcon(
+  element,
+  coords
+) {
+  return element.clientWidth - coords.x <= 25 + 2 * getPaddingRight(element);
+}


Suggestion: Add comment here explaining the math

Good callout. Updated for all such calculations: a628c53.

src/js/other-websites/icon.js

maxxcrawford · 2023-02-02T18:18:32Z

src/js/other-websites/icon.js

+  iframe.src = browser.runtime.getURL("inpage-panel.html");
+  iframe.style.position = "absolute";
+  iframe.style.width = `${RELAY_INPAGE_MENU_WIDTH}px`;
+  iframe.style.maxWidth = `95vw`;


Per my comment at the top of this review, I expect how the iframe is being initialized is where we would set the tip/rounded corners.

Hmm crap, in my spike I added this icon inside the iframe. I'll have to do some rework to be able to add it here, since you can't add ::before elements (or other children) to an iframe, so all the positioning will have to be redone to apply to a wrapper div. Hopefully I can find some time for that next week.

(Btw, this PR does already have rounded corners, and as far as I can see they're the same size -4px- as the current implementation?)

OK, I've struggled with making this appear properly for a bit now (in particular, getting it to take up the right space and have a consistent size), and I can't seem to make it work. My proposal would be to go without the chevron for now - it's relatively clear that the icon is related to the icon, so I think not having it for now would be better than having the current buggy icon implementation. I'm going to ask QA to test this.

maxxcrawford · 2023-02-02T18:20:49Z

src/js/other-websites/icon.js

+  const onUnderlayClick = (_event) => {
+    closeMenu();
+  };
+  underlay.addEventListener("click", onUnderlayClick);


Question/thought: Do we need any touchstart support for the Android version of this add-on?

I don't think so, as MDN mentions a bug in old Safari versions for click events, implying that they're supposed to fire, and StackOverflow says so. But I'm not sure how to verify - do you know if there's an easy way to test .xpi files on Firefox mobile?

maxxcrawford · 2023-02-02T18:39:12Z

src/js/other-websites/icon.js

+    `, linear-gradient(to left, #f68fff ${
+      getPaddingRight(element) * 2 + 25


Suggestion: Can you move #f68fff to a variable at the top of the file? I feel like this is hidden/buried CSS in this file that we'd want to better surface.

Extra suggestion: Move all/any CSS attributes to the top of the file.

Moved all colors in 3722328. Not sure if there were additional properties you were thinking of? (Note that most styles are dynamically generated based on existing styles for a particular input element, so I can't just move them to the top as hardcoded strings.)

Vinnl · 2023-03-10T13:13:41Z

@maxxcrawford I added cbde420 that fixes an issue where the spinner would never stop in Chrome - the commit message should explain more. Could you review that too, please? With that, I think that this might be ready to go.

With this method, we don't have to change the stacking order of the website, and thus should avoid a lot of issues where we break the website we're injecting in. It did require some extra work to stay accessible; we still inject a button that is visually hidden, but accessible to screen reader users and people who navigate using the keyboard. I've also modified the hover/focus state styling to make it easier. Note that I carried over the code in src/js/other-websites/icon.js from the experimental extension I prototyped this in, so the code style might be a little different from the rest of the codebase.

Whereas Firefox appears to unload the iframe when it is hidden, Chrome seems to preserve it. Unfortunately, our iframe contents currently assumes that it is unloaded (i.e. it only fetches data when it gets loaded, and never stops loading animations after inserting a mask), so when we kept the iframe in the DOM, that would cause issues in Chrome. Hence, this change ensures that the injected iframe gets destroyed whenever the menu is closed.

This re-applies 0447dca (from #499) to the new method of injecting the icon.

Vinnl · 2023-05-22T09:56:28Z

Rebased to clean up commits and resolve confilcts - specifically, I reapplied #499 in e06c3ae.

There were also 056a91e#diff-6d657968b51497d6080c30b3704a5e0cbac32a4707fb358dd33955f628c5d192 and 9b31315#diff-59a5db4e9a139708113dc0d0e60807e300b556f19ce611a5ffaa4a764c83f14b, but as far as I could see those were concerned with setting a particular height for the in-page popup, which IIRC should get determined automatically after this PR.

hecerinc · 2023-07-20T01:00:05Z

@Vinnl just checking if there's plans to merge this any time soon. Looks like you have this approved? Is there a timeline set for this?

Vinnl · 2023-07-28T09:54:16Z

@hecerinc I moved to work on Monitor and handed this over to other folks, but it looks like they haven't gotten around to releasing it yet. I'll bug some folks again to see if we can get this released :)

Vinnl added the web-compat This issue involves how the add-on interacts with a specific website label Jan 27, 2023

Vinnl requested review from maxxcrawford and codemist January 27, 2023 10:48

Vinnl self-assigned this Jan 27, 2023

Vinnl force-pushed the MPP-2510-icon-refactor branch from b67e3e5 to f8c64bc Compare January 27, 2023 10:48

github-actions bot deployed to review January 27, 2023 10:48 Active

Vinnl commented Jan 27, 2023

View reviewed changes

github-actions bot deployed to review January 30, 2023 13:38 Active

github-actions bot deployed to review January 31, 2023 13:43 Active

github-actions bot had a problem deploying to review February 2, 2023 14:06 Failure

Vinnl force-pushed the MPP-2510-icon-refactor branch from 197d3fe to 30b1e77 Compare February 2, 2023 14:10

github-actions bot deployed to review February 2, 2023 14:10 Active

github-actions bot deployed to review February 2, 2023 17:38 Active

maxxcrawford approved these changes Feb 2, 2023

View reviewed changes

github-actions bot deployed to review February 3, 2023 15:29 Active

github-actions bot deployed to review February 3, 2023 15:37 Active

github-actions bot deployed to review February 6, 2023 13:59 Active

Vinnl force-pushed the MPP-2510-icon-refactor branch from 25b1581 to 3722328 Compare February 6, 2023 14:11

github-actions bot deployed to review February 6, 2023 14:11 Active

github-actions bot had a problem deploying to review February 17, 2023 11:13 Failure

Vinnl force-pushed the MPP-2510-icon-refactor branch from febacc4 to b7990b6 Compare February 17, 2023 11:15

github-actions bot deployed to review February 17, 2023 11:15 Active

Vinnl force-pushed the MPP-2510-icon-refactor branch from b7990b6 to 3722328 Compare February 17, 2023 11:36

github-actions bot deployed to review February 17, 2023 11:36 Active

github-actions bot deployed to review March 10, 2023 13:11 Active

Vinnl mentioned this pull request Apr 18, 2023

Firefox and Chrome Extensions' login form field layering issue with captcha on accounts.nintendo.com/login #497

Open

Vinnl mentioned this pull request May 22, 2023

Google login shrinks typing area mozilla/fx-private-relay#1532

Open

Vinnl force-pushed the MPP-2510-icon-refactor branch from cbde420 to f9c73bf Compare May 22, 2023 09:08

github-actions bot deployed to review May 22, 2023 09:08 Active

Vinnl added 5 commits May 22, 2023 11:08

Don't disable scrolling with menu open

0230ff1

Move in-page menu to top if space below is tight

80554dd

Set background-image to SVG item via CSS instead of JavaScript

e06c3ae

This re-applies 0447dca (from #499) to the new method of injecting the icon.

Vinnl force-pushed the MPP-2510-icon-refactor branch from f9c73bf to e06c3ae Compare May 22, 2023 09:55

github-actions bot deployed to review May 22, 2023 09:55 Active

Vinnl mentioned this pull request Jun 23, 2023

Use client-only component for email input mozilla/blurts-server#3160

Closed

lloan merged commit bcbb044 into main Aug 11, 2023
3 checks passed

lloan deleted the MPP-2510-icon-refactor branch August 11, 2023 21:56

janbrasna mentioned this pull request Apr 22, 2024

Reduced z-index on email field wrapper #367

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change icon: overlaid button to background image #462

Change icon: overlaid button to background image #462

Vinnl commented Jan 27, 2023 •

edited

Vinnl Jan 27, 2023

Vinnl commented Jan 31, 2023 •

edited

maxxcrawford left a comment

maxxcrawford Feb 2, 2023

Vinnl Feb 3, 2023

maxxcrawford Feb 2, 2023

Vinnl Feb 3, 2023

maxxcrawford Feb 2, 2023

Vinnl Feb 3, 2023

Vinnl Feb 6, 2023

maxxcrawford Feb 2, 2023

Vinnl Feb 3, 2023

maxxcrawford Feb 2, 2023

maxxcrawford Feb 2, 2023

Vinnl Feb 3, 2023

Vinnl commented Mar 10, 2023

Vinnl commented May 22, 2023

hecerinc commented Jul 20, 2023

Vinnl commented Jul 28, 2023

		`, linear-gradient(to left, #f68fff ${
		getPaddingRight(element) * 2 + 25

Change icon: overlaid button to background image #462

Change icon: overlaid button to background image #462

Conversation

Vinnl commented Jan 27, 2023 • edited

New feature description

Screenshot (if applicable)

How to test

Checklist

Choose a reason for hiding this comment

Vinnl commented Jan 31, 2023 • edited

maxxcrawford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinnl commented Mar 10, 2023

Vinnl commented May 22, 2023

hecerinc commented Jul 20, 2023

Vinnl commented Jul 28, 2023

Vinnl commented Jan 27, 2023 •

edited

Vinnl commented Jan 31, 2023 •

edited