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

Added color preview box in tag browser sidebar #1779

Merged
merged 16 commits into from Sep 4, 2019

Conversation

@stefanb
Copy link
Contributor

commented Mar 5, 2018

Applied to the colour tags (https://wiki.openstreetmap.org/wiki/Key:colour) in the sidebar, having:

  • key either:
    • colour
    • building:colour
    • ref:colour
    • roof:colour
  • and value either:
    • 3 or 6 character hex code prefixed by "#", case insensitive (eg "#ffaabb" or "#fff" or "#FF0505") or
    • w3c colour name (case insensitive, eg "deeppink" or "DEEPPINK" or "DeepPink"...), full list from https://www.w3.org/TR/css-color-3/#svg-color

In such cases a 20px x 20px color preview box is floated to the right of the tag value field to be as unintrusive as possible, but still allowing the color preview and confirming the editors that the tag was entered correctly.

Example:
image

@tomhughes

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

Not sure I'm really the right person to review this given I don't think those tags belong in the database in the first place...

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2018

@@ -188,4 +190,16 @@ def telephone_link(_key, value)

"tel:#{value_no_whitespace}"
end

def colour_preview(key, value)
return nil unless (key =~ /^(|building:|ref:|roof:)colour$/ || key =~ /^(int_)?ref:colour(_(bg|tx))?$/) && !value.nil?

This comment has been minimized.

Copy link
@Zverik

Zverik Mar 6, 2018

Contributor

Maybe make it more generic? Taginfo shows many other colour tags, including three for seamarks and longer variants like building:roof:colour.

And int_ref:colour options are in hundreds, why make a clause specifically for these?

This comment has been minimized.

Copy link
@stefanb

stefanb Mar 6, 2018

Author Contributor

If we just look at the existing data and disregard the wiki, we should

Which makes the regex boil down to simple:

/colou?r/i

which can be understood that the editor wanted to tag some colour property and if the value indeed looks like a colour (hex code or valid w3c value) we can provide the preview.

Or we could require some common key structure. According to https://taginfo.openstreetmap
require some common key structure. According to https://taginfo.openstreetmap.org/search?q=colour we could generalize it to:

  • alow any optional prefix, separated by colon (:) or underscore (_)
  • allow any optional suffix, separated by colon (:) or underscore (_)
  • insist on british spelling with "u"
  • require lowercase
/^(.+[:_])?colour([:_].+)?$/

Both generalizations would allow us to preview all sorts of future colour tags, even unforeseen uses.

If we want to prevent that we would have to list the tags, and expand/maintain that with all approved tags in the future.

What level of generalization did you have in mind?

  • any tag or any tag that looks like it is related to colour
  • any structure or some specific separators
  • any spelling (british or us also)
  • any case (upper, lower, whatever)

This comment has been minimized.

Copy link
@gravitystorm

gravitystorm Mar 7, 2018

Collaborator

I'm very much opposed to anything that shows tagging mistakes as if they are correctly tagged. It's part of the guidelines established for the openstreetmap-carto project, and I'd like to apply similar guidelines to the browse pages:

"It's an important feedback mechanism for mappers to validate their edits and helps to prevent unfavorable fragmentation of tag use."

So no "color" tags and nothing that would show e.g. "buidling:colour" (note misspelling).

This comment has been minimized.

Copy link
@gravitystorm

gravitystorm Mar 7, 2018

Collaborator

(I've also removed an image from your comment - I know the joke, but it's not obviously a joke when shown out of context)

This comment has been minimized.

Copy link
@Zverik

Zverik Mar 7, 2018

Contributor

I agree that accepting color is a bit too much.

This comment has been minimized.

Copy link
@stefanb

stefanb Mar 7, 2018

Author Contributor

Yes, it is very important to indicate whether tagging was done correctly or not.... as much as possible.

The colour tag value is well defined and thoroughly checked and preview box is only shown if the value is a valid colour. User can then validate the colour correctness visually (editors could offering color pickers).

Key validation on the other hand is performed in the key column of the sidebar. Currently the only visual indicator is the presence of the link to wiki. That link is unfortunately missing for many valid keys (eg when key legitimately contains some variable or the wiki page is not existing or redirecting to a more general key). There might be also cases when the key was deprecated, but the link still exists because the page remained in wiki for reference. Key validity indication could be improved, to not just existence of wiki page but

  • also some other metadata from wiki (redirect, status...) or
  • link to most suitable wiki page (eg it the key is "building:levitating" which does not exist in wiki, just link to the first part "building" to the "key:building" wiki page which would give editors more hints about the topic and how to fix the tagging) or
  • OSM data (eg order tags by key occurance count)
  • some keys are only valid on specific elements (relations, ways, nodes),
  • some keys are requiring presence of related tags (eg drink:* tag requires appropriate shop or amenity tag)

Key validation is a complex topic to be tackled separately.

The question here is to what extent should the key validity influence the value validation, given that OSM has an evolving schema where no key can be "totally wrong", just maybe discouragd and less (or not at all) supported by some tools (renderers, routing engines...), and the community tries to reach consensus before introducing new keys, and educating newcomers of established schema.

Some minimal influence (key must at least contain "colour") is needed to prevent color preview boxes showing up occasionally in the name and other totally irrelevant tags.

It would not be clear to users to know why they don't see the colour preview box for their mispelled "buidling:colour=#aaa" tag. Absence of color preview box may say that something is wrong, but it would not point to the key, but would most likely leave user wondering whether should it be "#aaaaaa" or "lightgrey" or "lightgray" or "paledesertwhite" or ...?


I have played a bit with 100 most common keys sontaining "colour" and came up with a rather long regex:

/^(?>(?:roof|building(?>:(?>roof|facade))?|seamark:[a-z0-9_:]+|(?:cycleway:[a-z0-9_:]+:)?surface|(?>int_)?ref|destination|light):)?colour(?>(?>_(?>bg|tx)|:(?>arrow|back|bonnet|cap|text)))?$/

Which matches roughly 50% of these keys and some unrealistic fake ones (added by me at the end) - You can experiment yourself: http://rubular.com/r/nAjRBYjvUZ
(Please note that in the above regex the (?: ... ) and (?> ... ) are non-capturing and atomic-grouping performance optimisations, as we don't need individually matched components.)

Or we can stick to the KISS principle and use simple (cheap to execute and easy to maintain) regex:

/colour/

as the first filter and indication that user might have wanted to tag a colour value, and only then we try to interpret that given value as a colour.

This comment has been minimized.

Copy link
@Zverik

Zverik Mar 7, 2018

Contributor

Thanks for trying different options. The long regular expression looks interesting, but it is not future-proof. I'd propose to use /^(?:.+:)?colour$/ so that only colour or whatever:colour tags are matched.

I don't like variants with more suffixes, like colour_tx — that will catch wrong tags like :colour_pattern or apply only to local non-discussed tagging. Leading underscores are only used only in undocumented rare tags, so let's look only for semicolons.

Colour preview box: simplified key regex
and slightly reduced number of tests
@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2018

In 1fc4fb9 I have simplified the RegEx as suggested by @Zverik.

@Zverik
Zverik approved these changes Mar 8, 2018
@gravitystorm

This comment has been minimized.

Copy link
Collaborator

commented Mar 9, 2018

I'm still not keen on accepting arbitrary keys, as per my comment on the earlier diff. Yes, maintaining lists of "accepted" tags is hard, and not something that can be easily automated ("top 100" or "more than 100 uses" or "has a wiki page" etc). But such is life with OpenStreetMap, and editing software and rendering software has to make similar decisions, particularly if they are concerned (as I am) about reducing tag fragmentation and being part of the mapper feedback loop.

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2018

@gravitystorm, current solution is not "accepting" any key, it is merely taking a hint from the key that the editor might have tried to tag some colour and then visualizes it, if the value is a valid colour.

As I wrote above in the code reviewing discussion:

Key validation on the other hand is performed in the key column of the sidebar. Currently the only visual indicator is the presence of the link to wiki. That link is unfortunately missing for many valid keys (eg when key legitimately contains some variable or the wiki page is not existing or redirecting to a more general key). There might be also cases when the key was deprecated, but the link still exists because the page remained in wiki for reference. Key validity indication could be improved, to not just existence of wiki page but

  • also some other metadata from wiki (redirect, status...) or
  • link to most suitable wiki page (eg it the key is "building:levitating" which does not exist in wiki, just link to the first part "building" to the "key:building" wiki page which would give editors more hints about the topic and how to fix the tagging) or
  • OSM data (eg order tags by key occurance count)
  • some keys are only valid on specific elements (relations, ways, nodes),
  • some keys are requiring presence of related tags (eg drink:* tag requires appropriate shop or amenity tag)

Key validation is a complex topic to be tackled separately.

and from the UX perspective:

It would not be clear to users to know why they don't see the colour preview box for their mispelled "buidling:colour=#aaa" tag. Absence of color preview box may say that something is wrong, but it would not point to the key, but would most likely leave user wondering whether should it be "#aaaaaa" or "lightgrey" or "lightgray" or "paledesertwhite" or ...?

TL;DR: Key "validation" (or better: level of approval) should be indicated next to the key in the left column of the browser sidebar, and there is room for general improvement (not just relating to colour).


I can also make the regex accept exactly what you want, but that might impose some performance degradation and would require some more maintenance when new tags are approved. Current top 100 unique keys (with instance counts) containing "colour" are:

roof:colour=633618
building:colour=442670
colour=258229
seamark:buoy_lateral:colour=40222
colour:back=38630
colour:text=37170
seamark:light:colour=36357
colour:arrow=32554
seamark:topmark:colour=27711
light:colour=27586
seamark:beacon_lateral:colour=16125
building:roof:colour=14117
building:facade:colour=11607
seamark:light:1:colour=9389
ref:colour=7769
seamark:buoy_cardinal:colour=7437
seamark:buoy_cardinal:colour_pattern=7343
buoy:colour=6933
seamark:buoy_special_purpose:colour=6905
seamark:light:2:colour=6340
surface:colour=6313
colour:bonnet=6089
seamark:retro_reflector:colour=6078
seamark:beacon_special_purpose:colour=6053
colour:cap=5674
seamark:light:3:colour=3986
seamark:beacon_lateral:colour_pattern=3323
destination:colour=3245
seamark:light:4:colour=3161
seamark:topmark:colour_pattern=2921
seamark:daymark:colour=2862
destination:colour:lanes=2827
seamark:light:5:colour=2753
seamark:buoy_lateral:colour_pattern=2506
mitre:colour=2377
seamark:light:6:colour=2111
ref:colour_bg=1689
seamark:light:7:colour=1506
beacon:colour=1380
destination:colour:text=1047
seamark:light:8:colour=1044
ref:colour_tx=1032
destination:colour:back=1024
text_colour=989
topmark:colour=974
seamark:mooring:colour=796
seamark:light:9:colour=735
destination:colour:forward=735
destination:colour:ref=727
seamark:daymark:colour_pattern=705
seamark:beacon_isolated_danger:colour=702
destination:colour:backward=682
seamark:beacon_special_purpose:colour_pattern=667
seamark:buoy_safe_water:colour=667
seamark:buoy_safe_water:colour_pattern=645
seamark:beacon_cardinal:colour=624
seamark:beacon_cardinal:colour_pattern=565
topmark:colour_pattern=541
seamark:light:10:colour=485
destination:colour:lanes:forward=483
destination:colour:lanes:backward=432
seamark:buoy_isolated_danger:colour=427
seamark:buoy_isolated_danger:colour_pattern=399
destination:colour:arrow=395
seamark:buoy_special_purpose:colour_pattern=351
seamark:light:11:colour=335
seamark:beacon_isolated_danger:colour_pattern=284
cycleway:surface:colour=283
marker:colour=269
wall:colour=260
fire_hydrant:colour=257
seamark:landmark:colour=224
cycleway:right:surface:colour=215
memorial_plaque:colour=212
seamark:light:12:colour=207
display:colour=198
colour:ref=190
COLOUR_NU=189
destination:colour:int_ref=183
source:colour=172
building:colour:end=160
building:colour:start=160
destination:colour:back:lanes=144
colour_name=127
fence:colour=127
seamark:light:13:colour=125
int_ref:colour=112
int_ref:colour_tx=111
support:colour=107
colour:top=88
flag:colour=87
taxi:colour=86
lamp:colour=85
seamark:landmark:colour_pattern=85
cycleway:left:surface:colour=85
paving_stones:colour=82
trail_colour=80
destination:colour:ref:lanes=78
destination:colour:text:lanes=78
bonnet:colour=74

please note, that amongs these are many that are neither approved, nor even documented anywhere. Can you, or anyone else confidently decide for each of the listed 100 keys if it should be "approved" or not? Could a group of 5 people deciede unanimously on such a list? :)

@Zverik

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2018

Isn't validation better left to sites like Osmose and KeepRight? Maintaining them, updating to reflect new tagging schemes and generally improving is much easier than this one.

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

I have added a ticket #1782 to improve indicating the key correctness and linking to wiki in the key column of the browser sidebar.

Also ":colour" is a very general tagging suffix, that could be applied to many tags. Same as ":direction".

@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2018

Tried to test this coding locally, but somehow I'm getting escaped html. Not exactly sure where this comes from. Test on 701334b + merged stefanb/tag-colour-preview (1fc4fb9) into it.

grafik

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2018

@mmd-osm tnx for noticing. In 23fa748 i have fixed that by marking the html markup string as html_safe to prevent ruby from html escaping it.
It works:
image

...but rubocop complained with:

app/helpers/browse_helper.rb:80:183: C: Rails/OutputSafety: Tagging a string as html safe may be a security risk.
      %( <div class="colour-preview-box" style="background-color:#{h(value)}" title="#{h(t('browse.tag_details.colour_preview', :colour_value => colour_value))}"></div>#{h(value)} ).html_safe

@tomhughes, is it ok to add the exception to the whole file to .rubocop*.yml or is there a better way (adding exception for just this line or preventing the unwanted html escaping in some other way)? Did so in b3003a5

@tomhughes

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

@stefanb Well the issue isn't really disabling the cop, it's using html_safe! in the first place - that should be an absolute last resort to be used only if there is no way to do the job properly.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

To be clear in this case you can almost certainly do the job properly by constructing the HTML with ruby element helper methods instead of just embedding it as a string.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

On top of which it has inline styles (not acceptable and soon likely to be banned by security policy) and is using a div where I suspect a span is more appropriate given I doubt we're in a block context where that is being used.

Basically that whole line is a massive trigger.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

That's worse really - inline disablements clutter the code up.

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2018

All mentioned problems should now be fixed properly. Thanks for the valuable hints.
There is no sane workaround for inline style in this case, so that will have to be handled using a nonce during CSP implementation.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

Our CSP does currently allow inline styles but I think only because I haven't got around to tracking them all down and replacing them yet.

I'm not sure if a nonce will work - will need to look it up. There are certainly other ways though, like a data attribute that triggers javascript to set a style.

@HolgerJeromin

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

I'm not sure if a nonce will work - will need to look it up. There are certainly other ways though, like a data attribute that triggers Javascript to set a style.

Smells like a workaround for a unwanted lint error. Sorry, i was not aware that CSP is able to disallow online css.

@tomhughes

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

What are you on about? A CSP policy violation is not a "lint error".

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2018

But still, CSP has nonces for exactly such purposes to allow some specific use instead of general unsafe-inline directive.
Resorting to javascript is a possible workaround, but also has to be done properly (eg to not just eval() the data attribute :) )

And of course, it also depends on the framework's support for CSP and how easy it is to use nonces.

secure_headers gem seems to support this just fine, but as soon as the nonce is present in CSP style-src browsers will stop allowing unsafe-inline, breaking the styling in such places, so it needs to be done trough-out the website at once or have the CSP in report-only mode for a while until all instances are tackled.

Question: do you prefer javascript workaround or proper CSP implementation?

@tomhughes

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

I know the nonce will work in principle - the question is whether there is a good way for use to use one that doesn't negate the purpose. We almost certainly can't just commit the nonce in this repository for example as that would defeat the whole purpose.

So at the very least it probably needs to be a configuration option that can be set from chef. Even then I'm not sure that it helps much given a bad actor can just access the source and copy the nonce.

@mmd-osm

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

After some testing, the new colours somehow feel a bit, well how should I put it, annoying. Please don't get me wrong, I still find the idea cool, yet it could be made a bit less obtrusive, i.e. giving a user some indication there's some colour without adding too much distraction.

The only idea I could come up with was to use different opacity value depending on where the mouse is. This may be a stupid idea to start with, and add even more confusion for a normal user. Maybe others don't find it distracting as it is right now?

grafik

Opacity value of 0.1 instead of 0.4:

grafik

When focusing on the key/value pair block, the colours are shown with opacity 1 again, like before.

grafik

--- a/app/assets/stylesheets/common.scss
+++ b/app/assets/stylesheets/common.scss
@@ -1219,9 +1219,17 @@ tr.turn:hover {
       margin: 0px;
       border: 1px solid rgba(0, 0, 0, .1); 
       // add color via inline css on element: background-color: <tag value>;
+
+      transition: opacity 0.1s ease-out;
+      opacity: 0.4;
     }
   }
 
+  .browse-tag-list:hover .colour-preview-box {
+     display:block;
+     opacity: 1;
+  }
+
   .warning {
     margin: 0 0 $lineheight/2 0;
     padding: 0 $lineheight/2;
@HolgerJeromin

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

Perhaps make the div smaller.
Opacity changes the color. On mobile we have no :hover

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2018

@tomhughes: "nonce" = number used once, not a long term secret configured somewhere in chef

@mmd-osm: you made a good point with overly-intrusive colours, but in reality most building colours are not as vivid/saturated as in our test cases, but usually much more pale. Having it shown more pale than the actual value might encourage users to enter more aggressive/vivis/saturated colours, so that they would look more accurate in the browser even without hovering.

@HolgerJeromin: valid point about mobile not having hover, here is a sample with smaller boxes: size 12x12px, same as 12px font in the table, with 4px margin on top and bottom to keep them vertically centered:
image
Would that be better?

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2018

Replaced the inline style attribute with data attribute + javascript (so that it won't make implementation of strict CSP any harder) and made the boxes smaller (same size as text and screenshot in my previous comment)

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

@tomhughes, any other concerns?

@tomhughes tomhughes force-pushed the openstreetmap:master branch from 0e51259 to 6142980 Oct 31, 2018

@gravitystorm

This comment has been minimized.

Copy link
Collaborator

commented Aug 28, 2019

I've been reviewing this PR today, and despite my unresolved objections I'm willing to go ahead and accept this PR.

However, in the intervening time the PR has bit-rotted and now has conflicts, mainly due to the split of browse_helper and browse_tags_helper last year. @stefanb if you're willing to rebase this onto a recent master, that would be helpful. If not, no worries, I can do that in a few weeks time.

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

@gravitystorm thanks for picking this up, I can check it once i restore my OSM dev environment on a rainy day :)

stefanb added 5 commits Aug 31, 2019
Colour preview moved into new browse_tags_helper
In 74d2c43 browse_helper was split int two
@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@gravitystorm, I have resolved the conflicts and fixed the code style problem that appeared with changed / more strict linting during the building process.
Thanks for pointing out the cause of the conflicts.
Please review and test it.

@gravitystorm gravitystorm merged commit 374668c into openstreetmap:master Sep 4, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 87.95%
Details
@gravitystorm

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

@stefanb Great, thank you! I've merged this now. I look forward to seeing it on real-life data, to see if there's any iterations we need to make.

Thanks again for rebasing and making the linting changes to go with it.

@stefanb

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Thanks for checking, merging and deploying!

Some examples:
https://www.openstreetmap.org/way/176487479 (building)
https://www.openstreetmap.org/relation/59655 (road route relation)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.