Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop custom file upload plugin in favor of CSS solution #31955

Merged
merged 3 commits into from
Oct 28, 2020

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Oct 22, 2020

CSS only alternative solution for #31085.
Advantages:

  • No js dependencies
  • No need to add translated button labels
  • Labels are automatically adjusted when with <input type="file" multiple>

TODO:

  • Wait until legacy Edge is dropped This isn't really necessary, since legacy Edge already has a decent fallback
  • Extend migration guide
  • Check if the alpha version is correct
  • Fix border radii on iOS

https://deploy-preview-31955--twbs-bootstrap.netlify.app/docs/5.0/forms/form-control/#file-input
https://deploy-preview-31955--twbs-bootstrap.netlify.app/docs/5.0/forms/validation/#supported-elements

Closes #30265
Closes #31085

@mdo
Copy link
Member

mdo commented Oct 22, 2020

Thoughts on adding cursor: pointer here? I know it's not required, but compared to .form-control on textual inputs, where the cursor changes to indicate interactivity, there's no change here.

@patrickhlauke
Copy link
Member

Thoughts on adding cursor: pointer here? I know it's not required, but compared to .form-control on textual inputs, where the cursor changes to indicate interactivity, there's no change here.

this old chestnut again ;)

as we do it on buttons by default, sure. also worth adding the <input type="color"> ?

@MartijnCuppens
Copy link
Member Author

Thoughts on adding cursor: pointer here?

Fine by me. Only on the button or the whole input?

@patrickhlauke
Copy link
Member

everything that's clickable, so the whole thing i'd say

@patrickhlauke
Copy link
Member

also, good work on this btw. looks/works nicely

@MartijnCuppens
Copy link
Member Author

Ok, should we add hover states? If yes, what kind of hoverstate? Shading the button background color maybe?

@MartijnCuppens MartijnCuppens force-pushed the main-mc-form-file branch 2 times, most recently from 44f5e88 to 569c764 Compare October 22, 2020 17:46
@MartijnCuppens
Copy link
Member Author

I've added a subtle hover state

@patrickhlauke patrickhlauke self-requested a review October 22, 2020 19:47
@mdo
Copy link
Member

mdo commented Oct 23, 2020

@MartijnCuppens this is amazing. Love the little hover state.

@MartijnCuppens MartijnCuppens changed the title Drop custom file upload plugin in favor of CSS Drop custom file upload plugin in favor of CSS solution Oct 24, 2020
@MartijnCuppens MartijnCuppens marked this pull request as ready for review October 24, 2020 19:14
@alecpl
Copy link
Contributor

alecpl commented Oct 25, 2020

There's some padding issue in Firefox 82 on Ubuntu.
input-bug

@MartijnCuppens
Copy link
Member Author

@alecpl,
Nice catch
That seems to be caused by these 5px:
https://searchfox.org/mozilla-central/rev/0b7007a23bc16c857f894140e12f307bfeef2fdd/layout/style/res/forms.css#494

I've now added a -moz-margin-end property to compensate these 5px.

@ffoodd
Copy link
Member

ffoodd commented Oct 27, 2020

@MartijnCuppens Doesn't seem to change anything, sadly. But maybe changing it in the devtools is not the best way?

@alecpl
Copy link
Contributor

alecpl commented Oct 27, 2020

I've found a fix: .form-control::file-selector-button { font-family: inherit; }. It can be also font-family: unset.

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this PR! :D

@mdo
Copy link
Member

mdo commented Oct 27, 2020

Also going to cross reference that #31993 fixes the issues @alecpl mentions here.

@alecpl
Copy link
Contributor

alecpl commented Oct 28, 2020

@mdo, #31993 does not fix my issue, need the font-family:inherit fix mentioned in my previous comment.

A few more words about the issue. Looks like firefox does not inherit font-family on the file input button from the input. It needs to be told to do so explicitely. Without the fix the computed font-family is "Noto Sans" on my computer. This font's line-height is 1px higher than what I get when I add the inherit rule.

@ffoodd
Copy link
Member

ffoodd commented Oct 28, 2020

Very nice finding @alecpl, I confirm it solves the issue.

We should find some reference to document this behaviour, either a bugzilla link or something… The MDN page for ::file-selector-button does not mention this but made me notice the need for ::-webkit-file-upload-button version too: does Autoprefixer handles this one?

Edit: x-ref Bugzilla

@XhmikosR
Copy link
Member

XhmikosR commented Oct 28, 2020 via email

@ffoodd
Copy link
Member

ffoodd commented Oct 28, 2020

FWIW the bug I filled against Firefox got a very clear explanation (mainly, this pseudo-element should look like a button like it always did, not like an input).

I guess this "fix" @alecpl found could move to our reboot, and maybe suggested to Normalize and friends.

@MartijnCuppens
Copy link
Member Author

We should find some reference to document this behaviour

It's shadow DOM, so all properties are reset to the default behaviour. So basically we should keep in mind these things need to re-reset (font size seems to be inherit by default though):

bootstrap/scss/_reboot.scss

Lines 431 to 434 in 3e2f9ab

margin: 0; // 1
font-family: inherit;
@include font-size(inherit);
line-height: inherit;

#31993 seems to be unrelated, for the issue @alecpl reported, but does seem to fix issues with errors in rounding when zoomed in/out which resulted in the same issue visually.

Thanks for the fix @alecpl, this was indeed needed, not only for FF, but also for Chrome (see https://codepen.io/MartijnCuppens/pen/yLJpmvE?editors=1100)

@MartijnCuppens
Copy link
Member Author

Hmm yeah, maybe we should indeed move this to reboot, I'll fix that

@MartijnCuppens
Copy link
Member Author

Ok, fixed the font-family (and all other font related properties) inheritance and also reported the 5px thing with Firefox, so we might be able to remove that dirty fix we now have in the future (https://bugzilla.mozilla.org/show_bug.cgi?id=1673895).

@ffoodd
Copy link
Member

ffoodd commented Oct 28, 2020

It's shadow DOM, so all properties are reset to the default behaviour.

Not really, as you may see in Firefox sources I linked earlier: https://hg.mozilla.org/mozilla-central/rev/ff6c38ffa425#l7.104 — only a few properties are set (and for example ::placeholder does not require anything to inherit the font).

But that was fun to search, and a very nice PR! Great job @MartijnCuppens 💯

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: when ready!

@MartijnCuppens MartijnCuppens merged commit b1c7d1d into main Oct 28, 2020
@MartijnCuppens MartijnCuppens deleted the main-mc-form-file branch October 28, 2020 17:29
@MartijnCuppens
Copy link
Member Author

Shipped 🚢

I'll keep an eye on https://bugzilla.mozilla.org/show_bug.cgi?id=1673895 if FF is planning to change how they set their spacing

@Johann-S
Copy link
Member

That's amazing @MartijnCuppens nice work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bs-custom-file-input doesn't work anymore since the form updates
7 participants