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

Fixxes display of SiteUrl placeholder #13036

Closed
wants to merge 3 commits into from
Closed

Fixxes display of SiteUrl placeholder #13036

wants to merge 3 commits into from

Conversation

fdellwing
Copy link
Contributor

fixes #13035

@fdellwing
Copy link
Contributor Author

There are a lot of tests failing and some very strange screenshots especially this two:

https://builds-artifacts.matomo.org/matomo-org/matomo/3.x-dev/28135/UIIntegrationTest_dashboard1.png
https://builds-artifacts.matomo.org/matomo-org/matomo/3.x-dev/28135/Morpheus_load.png

I'm pretty sure this has nothing to do with me, but I will not sync my screens until this is worked out.


But please explain this: https://builds-artifacts.matomo.org/matomo-org/matomo/3.x-dev/28135/MeasurableManager_add_measurable_view.png

How could the thing I fixxed be already expected and why is it getting rendered wrong?

Just to be clear here: This fix is against the W3C standard HTML 5.2 from December 2017! As it clearly states:

The placeholder attribute represents a short hint (a word or short phrase) intended to aid the user with data entry when the control has no value. A hint could be a sample value or a brief description of the expected format. The attribute, if specified, must have a value that contains no U+000A LINE FEED (LF) or U+000D CARRIAGE RETURN (CR) characters.

Source: https://www.w3.org/TR/html5/forms.html#the-placeholder-attribute

I tested this change successfully in: IE11, FF60, Chrome 66

@Findus23
Copy link
Member

Findus23 commented Jun 5, 2018

How could the thing I fixxed be already expected and why is it getting rendered wrong?

I can explain this: It is already working correctly with the spaces if the windows is narrow enough.

@diosmosis
Copy link
Member

Thanks for creating this PR @fdellwing! Looks like the reason it fails in phantomjs is because it doesn't work in webkit. See, eg, this screenshot in Safari:

image

Also, I'm not sure if .invalid is a good suffix to use, I think it may confuse some users. siteUrl.example might be better. Or maybe just example1.com?

@fdellwing
Copy link
Contributor Author

Thats bad news, then there is not really a nice way to do this. I would suggest using JS to write the placeholder into the field and removing it if the input gets focus. Sadly I do not have time for this atm.

You can decline this PR if you are more happy with the current version.

@Findus23
Copy link
Member

@fdellwing What about the more hackish solution of just adding more spaces and replacing the domain with an .example one?

@fdellwing
Copy link
Contributor Author

Thats not hackish but simple ugly, but yes, I could do that.

Fabian Dellwing added 2 commits June 22, 2018 09:41
@fdellwing
Copy link
Contributor Author

I just tripled the current spaces for now, can someone check this with an UHD resolution? For 4k we would need even more I think.

@sgiehl
Copy link
Member

sgiehl commented Jun 25, 2018

As we are building those inputs with angular based on the definitions, I'm wondering if we maybe should do the required magic for safari in angular. Afaik all other modern browsers are able to show the newline in textareas. So maybe we could implement a check in JS if the browser "supports" it or not. And if not, replace the newline with whitespaces.
This way we would have a "normal" definition with new lines. And we maybe can someday simply remove the whitespace hack if apple decides to support w3c definitions fully.

@fdellwing
Copy link
Contributor Author

I have no clue of angular, so sadly you have to do that yourself.

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

Successfully merging this pull request may close these issues.

Placeholder for SiteUrl is displayed incorrectly
4 participants