1514567 - Browser-style for input[type=text]:invalid missing

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[rugk]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

Create the following options HTML:

<li class="browser-style">
	<label for="insertHandle">Label</label>
	<input type="text" id="insertHandle" name="…" pattern="^@?([^@ ]+)@([^@ ]+)$">
</li>

The "browser-style" class is applied as per https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/user_interface/Browser_styles.
So the :focus pseudo-class works and hi9ghlights it in nice Photon colors.
However, the element has a RegEx pattern, and thus it should also properly be styled with the :invalid class. (as per https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/HTML5/Constraint_validation#Controlling_the_look_of_elements)


Actual results:

So if I enter an invalid input (in my case: an input like "jkjkj" without @) it still shows this focus style.


Expected results:

As when you leave out "browser-style", it should show some red border or so… 

Actually this is the behavior when you do not specify the "browser-style": In forms.css:790 the inspector shows me that there is CSS for that:

:not(output):-moz-ui-invalid {
  box-shadow: 0 0 1.5px 1px red;
}

This is however overwritten by "browser-style" and it may also not fit into the design.

Comment 1

[rugk]

A workaround or probably solution is to add something like this:

.browser-style > input[type=text]:invalid:not(:focus) {
  border-color: var(#d70022);
  box-shadow: 0 0 0 2px rgba(215, 52, 77, 0.75);
}

I've adapted the style to https://design.firefox.com/photon/patterns/errors.html as soon as I could without exact numbers or so

Comment 2

[rugk]

Forget to remove var, like this, obviously:

.browser-style > input[type=text]:invalid:not(:focus) {
  border-color: #d70022;
  box-shadow: 0 0 0 2px rgba(215, 52, 77, 0.75);
}

Comment 3

[rugk]

P5 is you accept contributions, yeah?

I could give it a try, but I don't know how it is (exactly) supposed to look like. I mean, I gave an example above, but I don't know whether it is what is exactly wanted.

Comment 4

[Caitlin Neiman [:caitmuenster]]

Hey rugk, sure! You should take a look at the onboarding documentation to get set up: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Luca (:rpl) will be mentoring this bug and can answer more specific questions.

Comment 5

[Caitlin Neiman [:caitmuenster]]

Hey Blake! Could you advise on what the fix is supposed to look like?

Comment 6

[Blake Winton (:bwinton) (:☕️)]

It should be basically what rugk specified in comment #2. The exact colours and styling are listed in https://design.firefox.com/photon/components/input-fields.html#behaviors but they're very close.

(The one other comment I have is that it would be nice to use var(--red-60) instead of #d70022, if you can… 🙂)

Comment 7

[Caitlin Neiman [:caitmuenster]]

Hey rugk, how's it going with this bug?

Comment 8

[rugk]

Wait, you are assigning the wrong needsinfo, I guess…

So I tried to adjust it with Janitor, but it totally fails, because the janitor build does not load the whole extension.css (even without modifications/everything commented) in thus it is displayed correctly in Firefox. (as the the general rule from forms.css takes effect)
I guess a screenshot explains it better: https://hostux.pics/images/2019/01/24/grafikde79fc4387867530.png
(also note e.g. that the "reset button" below also uses "browser-style", but as you can see also misses it's style. So there is really something wrong.)

And yes, this is my add-on and I've disabled my own workaround styles in the inspector window.

Build ID of Firefox is 20181201144611.

Also I noticed that the whole extensions.css does not use the variables (you mentioned), so I can easily fix it. But unless this thing works in Janitor again, I can hardly test it.

I can give you access to the container if you want (and if that works in Janitor).

Comment 9

[rugk]

Attachment: Bug 1514567 - Added :invalid CSS style for extension's input elements

Before this change no style for invalid input fields was included in Firefox and thus invalid text fields where not highlighted as invalid as per the Firefox Photon design guide.

Comment 10

[rugk]

Okay, now used a new container (and another one due to issues with the git thing in janitor, so using hg, but well… that's off-topic), so now it works.

BTW, I am very confused who is now mentoring this bug. You've added a mail above, then mentioned a different(?) username as a mentor and finally none of these two seem to have left any comment in this bug. So I've just added all ones I assumed, who may be doing this.

Comment 11

[rugk]

BTW also tried to replace the hardcoded colors with var's, but could not, as they are just made up and not defined in the Firefox photon guide, actually (or the guide is newer, whatever). Opened bug 1522697 for this.

Comment 12

[Caitlin Neiman [:caitmuenster]]

Hey rugk, you are right -- I needinfo'd the wrong person. :) Thanks for the catch, and the patch!

Luca (:rpl) is still mentoring this bug. We needed to consult with Blake, who's on the UX team, to confirm the end state requirements.

Luca will be able to review your patch on Phabricator within the next couple of days (I can't commit him to a specific time, but I do know that he is very responsive :) ). He'll let you know what the next steps are.

Thanks again!

Comment 13

[Narcis Beleuzu [:NarcisB]]

:rugk , I can`t land this patch because of the following error
"Diff does not have proper author information in Phabricator. See the Lando FAQ for help with this error. "
https://wiki.mozilla.org/Phabricator/FAQ#Lando

Comment 14

[rugk]

No, no, no… wtf, no…
Could not someone or some tool (I mean, I used all of them arc, git, hg, …) tell me this before (it is going to be merged into master)??

I certainly don't want to resubmit the commit. I made it with Janitor, so possibly I still have the container, but if not, that is going to be hard.

So what shall I do now?
Can't you just manually add author info, please? (Here you can get some author info to add: https://privatebin.net/?adba17fa70657b51#RUU2rJEp0vU5a8UdeAF/J7rTegJOcAsfabQZYlTS6Vk= [attention: one time link!).)

Comment 15

[rugk]

Okay, luckily my container was still there, so I checked it and it is actually configured, see: https://vim.cx/?ede76a907e4e4683#xhQN8H/HfALnn3/+eU/lxkBoQkbfaKtQWTXz4xfRabo=

I think the issue is that only a mail address is given, no username.

Comment 16

[rugk]

Okay, updated thing in Phabricator, hopefully it works now.

Comment 17

[Cosmin Sabou [:CosminS]]

This revision is blocked from landing.

Diff does not have proper author information in Phabricator. See the Lando FAQ for help with this error. https://wiki.mozilla.org/Phabricator/FAQ#Lando

Comment 18

[rugk]

Oh no, I tried to fix adding that info already and it seem to have failed. I don't want to recreate that commit, really…

So can't you adjust/set it manually?

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09c7b0032dac
Added :invalid CSS style for extension's input elements. r=bwinton

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/09c7b0032dac

Comment 21

[Caitlin Neiman [:caitmuenster]]

Woohoo, glad that this landed! 🎉 Thanks for the patch, rugk. We've added your contribution to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you.

Comment 22

[Madalin Cotetiu]

Attachment: BrowserStylePattern.png

Verified in Win7x64, Ubuntu 14.0.4x32, MacOS 10.14.1 on FF67.0b5 (20190325125126)

I have created a test webextension using the HTML code in comment 0 and when the string entered in the input field does not match the pattern the field has a red border. (Please view the attachement)

Can you guys confirm that this is the expected behaviour so we can mark this bug as verified fixed?

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Looks good to me! 🙂

Comment 24

[Madalin Cotetiu]

Thanks for the fast response! Marking the bug as verified fixed

Comment 25

[rugk]

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you.

Thanks, yes. Actually, I should already have one. :)

https://mozillians.org/de/u/rugk/

But wait a moment, I cannot login there, because you also switched to Auth0 there again and this is… arggg… nooo… can't you stop this Auth0 thing.
Here is my problem: https://discourse.mozilla.org/t/auth0-creates-new-account-when-github-mail-changes/36362/6?u=rugkx

Comment 26

[Caitlin Neiman [:caitmuenster]]

(In reply to rugk from comment #25)

But wait a moment, I cannot login there, because you also switched to Auth0 there again and this is… arggg… nooo… can't you stop this Auth0 thing.
Here is my problem: https://discourse.mozilla.org/t/auth0-creates-new-account-when-github-mail-changes/36362/6?u=rugkx

You're vouched! Sorry to hear you are having problems with Auth0. :/ Henrik should be able to help out over on bug 1540407 (but please feel free to ping me if it's not moving forward).

Comment 27

[rugk]

Thanks. I've now just set up my profile again there (IIRC I had no vouches before anyway.) Also added an alternative login method, so hopefully it won't be deleted again.

So thanks for vouching. BTW this was not my first code contribution, actually my first one was this one. :)