fx-private-relay-add-on icon indicating copy to clipboard operation
fx-private-relay-add-on copied to clipboard

High z-index value for email field wrapper causes stacking issues

Open alexgibson opened this issue 4 years ago • 9 comments

The relay-email-input-wrapper element that gets added around email input fields has the following CSS z-index value:

z-index: 99999999999999999;

I'm sure this property is likely needed for a reason, but its value is way too high. This results in the email field appearing visually stacked on top of pretty much any other element that may be on a website.

For example, sticky navigation:

image

alexgibson avatar May 01 '20 08:05 alexgibson

@lesleyjanenorton I believe this is the underlying issue for a number of other website compat bug reports where the field is on top of other page elements?

groovecoder avatar Jul 22 '20 16:07 groovecoder

completely true. Very annoying. z-order has an impact on UX.

humpfhumpf avatar Aug 16 '20 10:08 humpfhumpf

Chiming in on this issue because it’s the earliest reference I can find to this particular bug, but it’s been mentioned elsewhere too. Setting a z-index of 9999999999999999999999 (!??) is… much too much.

I see some PRs have tried to address this and have been closed without merging. I’d be happy to open a new PR pulling the various z-indexs back into the stratosphere. Is such a PR likely to get merged? I don’t want to waste anyone’s time if another solution is incoming.

Uninstalling Relay until this is fixed; it breaks a ton of websites and feels like bad web-citizenry.

rileyjshaw avatar Mar 06 '21 05:03 rileyjshaw

I'm still experiencing the same issue on Firefox 95.0 and the latest version of Nightly (97.0, 2021-12-20) with this extension.

https://user-images.githubusercontent.com/17863082/146827395-4a04feda-b6bb-44c8-9342-20e133120e38.mp4

lolrepeatlol avatar Dec 20 '21 20:12 lolrepeatlol

This is still present today. This addon breaks pages - come on, this is a major issue, why is it lying around for so long!

Paratron avatar Jan 21 '22 08:01 Paratron

Also happens on https://accounts.nintendo.com. The email input is displayed on top of the CAPTCHA:

accounts nintendo com

Does not occur if Relay extension is disabled (but 1password extension enabled)

jwhitlock avatar May 31 '22 15:05 jwhitlock

I created a PR to fix this lingering issue: https://github.com/mozilla/fx-private-relay-add-on/pull/367 Would love to have someone from the Relay team review sometime soon!

reemhamz avatar Jun 23 '22 01:06 reemhamz

Two. Years. This is so embarrassing.

Paratron avatar Jun 23 '22 04:06 Paratron

I took a look at the 1passwrod extension to see how they handle this, and whilst they also use a very high z-index value, they add some clever additional behaviour to help minimise the issue:

  1. The icon is only displayed next to the input when the input is focused.
  2. The icon will hide / disappear again if the user scrolls the web page.

I think adding the two above items would likely help hide this issue in the majority of use-cases mentioned above.

alexgibson avatar Jun 23 '22 09:06 alexgibson

Hi @alexgibson I would like to comment on this, which I assume affects many people using Relay, including me.

I've looked at how other extensions do this, and one extension in particular, Kee (Password Manager) (source here) does it in an interesting way so overlay issues don't really happen:

  • Setting the background image of the input field to Relay's icon via style setters in JS
  • Adjusting the size, position, etc to align at the right at an appropriate dimension.
  • When the user hovers, it checks if it exceeds the threshold then based on that it shows the pointer icon, else the regular icon.
  • Using the same check, display the overlay if user is detected clicking on the icon (based on threshold), else nothing is performed.

This should eliminate the need for the z-index as well as the input overlay. The relevant file is here: https://github.com/kee-org/browser-addon/blob/8ae4261c82578ea0f08f6ffe1409145872a81c96/page/keeFieldIcon.ts (in TypeScript), in the addIcon function.

Based on their code, I have implemented a rudimentary version on Relay. (Note: the code below has some errors, but nonetheless shows a PoC of the new method) I changed the addRelayIconToInput function in src/js/other-websites/add_input_icon.js to this

async function addRelayIconToInput(emailInput) {

  emailInput.style["background-image"] =  "url('https://relay.firefox.com/_next/static/media/favicon.fc247257.svg')"
  emailInput.style["background-repeat"] =  "no-repeat"
  emailInput.style["background-attachment"] =  "scroll"
  emailInput.style["background-size"] =  "16px 16px"
  emailInput.style["background-position"] =  "calc(100% - 4px) 50%"
  emailInput.style["cursor"] =  "auto"
  emailInput.addEventListener("mousemove", async (e) => {
    if (!e.isTrusted || e.target.disabled) return

    const rect = e.target.getBoundingClientRect()
    const leftLimit = rect.left + rect.width - 22
    // console.log(leftLimit)
    // console.log(e.clientX)

    if (e.clientX > leftLimit) {
      e.target.style["cursor"] = "pointer"
      // console.log("left")
      return;
    }
    // console.log("not")
    e.target.style["cursor"] =  "auto"

  })

  //
  const sendInPageEvent = (evtAction, evtLabel) => {
    sendRelayEvent("In-page", evtAction, evtLabel);
  };

  emailInput.addEventListener("click", async (e) => {
    if (!e.isTrusted) {
      // The click was not user generated so ignore
      return false;
    }

    const bcrect = e.target.getBoundingClientRect();
    const leftLimit = bcrect.left + bcrect.width - 22;
    if (e.clientX > leftLimit && bcrect.top <= e.clientY && e.clientY <= bcrect.bottom) {
      lastClickedEmailInput = emailInput;
      sendInPageEvent("input-icon-clicked", "input-icon");

      preventDefaultBehavior(e);
      window.addEventListener("resize", positionRelayMenu);
      window.addEventListener("scroll", positionRelayMenu);

      const signedInUser = await isUserSignedIn();

      const relayInPageMenu = buildInpageIframe({isSignedIn: signedInUser});
      const relayMenuWrapper = createElementWithClassList(
          "div",
          "fx-relay-menu-wrapper"
      );

      // Close menu if the user clicks outside of the menu
      relayMenuWrapper.addEventListener("click", closeRelayInPageMenu);

      // Close menu if it's already open
      // relayIconBtn.classList.toggle("fx-relay-menu-open");

      // if (!relayIconBtn.classList.contains("fx-relay-menu-open")) {
      //   return closeRelayInPageMenu();
      // }

      if (!signedInUser) {
        addRelayMenuToPage(relayMenuWrapper, relayInPageMenu, emailInput);
        sendInPageEvent("viewed-menu", "unauthenticated-user-input-menu");
        return;
      }

      sendInPageEvent("viewed-menu", "authenticated-user-input-menu");
      addRelayMenuToPage(relayMenuWrapper, relayInPageMenu, emailInput);

    }

  });

  // divEl.appendChild(emailInput);
  // emailInputWrapper.appendChild(divEl);
  sendInPageEvent("input-icon-injected", "input-icon");
}

A screenshot of the proposed change: image Currently, the offsets for the overlay menu when clicked are not set properly but if I do a PR I'll fix them.

I can push a PR if you'd like/this change is acceptable, in which I will try to fix the issues with the current code and optimise it, delete unnecessary code, etc, and possibly re-implement it based on the same idea to avoid license issues.

Please do let me know if you'd like me to do so.

This may require a sizeable refactor to the code base though.

Thanks!

czlucius avatar Jan 05 '23 16:01 czlucius

Dude, three years now. I am unsubscribing from this issue - it wont ever get fixed 😓

Paratron avatar Jan 05 '23 16:01 Paratron

Thanks @czlucius, I actually have a prototype working with exactly that approach*, and it does appear to work well so far. Unfortunately it does require some more work to get that over the finish line, among which testing it on a list of commonly-used websites, which we have yet to find the time for, and, as you mentioned, a sizeable refactor of the codebase. So progress is being made, but it is slow, unfortunately.

 * Well, roughly that approach - some extra work is needed to ensure that people with visual or motor impairments can use it, but I've got solutions for that too.

Vinnl avatar Jan 06 '23 08:01 Vinnl

Oh I see. That's good (I mean for the progress you've made to get it to the stage of a working prototype). May I help out in that?

czlucius avatar Jan 06 '23 14:01 czlucius

Not much I think, unfortunately. The main thing is that the team needs to review and get familiar with the refactored codebase so they can build on top of it, so we need time allocated there - which of course is competing with all the other things people are clamouring for. Sorry about that - personally, I'm really hoping we can get this done soon, but of course, it's not just up to me.

Vinnl avatar Jan 06 '23 14:01 Vinnl

Ok I hope the team(at Relay) would be able to get this ready soon. Thanks

czlucius avatar Jan 06 '23 16:01 czlucius