Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#21459 closed defect (fixed)

Make atlas fingerprint selectable by double-clicking

Reported by: teor Owned by: irl
Priority: Medium Milestone:
Component: Metrics/Relay Search Version:
Severity: Normal Keywords: regression
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When I use atlas on macOS 10.12 with the default Tor Browser window size (on a 1440 x 900 display), the relay fingerprint is wrapped across two lines.

This means I can no longer select the fingerprint by double-clicking.
(I only get the first 37 characters.)

Can you please fix this regression?
It worked fine in earlier versions.

Child Tickets

Change History (14)

comment:1 Changed 3 years ago by cypherpunks

The fingerprint got wrapped in #12685 and subsequently in #21350 using a different solution. Indeed this wrapping is what prevents double-clicking from selecting the entire fingerprint but triple-clicking still seems to work.

comment:2 Changed 3 years ago by RaBe

Status: newneeds_review

Here's a small patch that shows the fingerprint in one line in the default tor browser window size and always selects the whole fingerprint on a single click even if it's displayed as multi line:

https://github.com/RaphaelBergmann/atlas/commit/0c8ed21dac97beb9e13b0526c1fab303561d5b95

comment:3 in reply to:  2 Changed 3 years ago by cypherpunks

Status: needs_reviewneeds_revision

Replying to RaBe:

Here's a small patch that shows the fingerprint in one line in the default tor browser window size and always selects the whole fingerprint on a single click even if it's displayed as multi line:

https://github.com/RaphaelBergmann/atlas/commit/0c8ed21dac97beb9e13b0526c1fab303561d5b95

In that commit the fingerprint is changed from a dd to a dt element. Was this intentional? My testing finds that the user-select CSS property (nice find btw) also works on dd elements so there is no need to change it and would make the introduction of the fingerprint CSS class unnecessary.

comment:4 Changed 3 years ago by cypherpunks

You also introduce some trailing spaces in the force-select class.

comment:5 Changed 3 years ago by RaBe

Status: needs_revisionneeds_review

We still need the fingerprint class to adjust the DDs margin, but I agree keeping it as a DD might be the better way :) Also I removed the trailing spaces.

https://github.com/RaphaelBergmann/atlas/commit/0a8aefbe54968e2e6aa302ad4dad4762c7ad77f6

comment:6 in reply to:  5 ; Changed 3 years ago by cypherpunks

Replying to RaBe:

We still need the fingerprint class to adjust the DDs margin, but I agree keeping it as a DD might be the better way :) Also I removed the trailing spaces.

https://github.com/RaphaelBergmann/atlas/commit/0a8aefbe54968e2e6aa302ad4dad4762c7ad77f6

I don't see why the margins of the fingerprint field should be altered. It requires custom code (which need to be maintained) and makes the look of the fingerprint inconsistent with other fields that use <pre>. Trying to get the fingerprint to use a single line is an unreachable goal because you don't know the sizes of all the viewports that are used. (For instance, on my end your branch still doesn't make the fingerprint use a single line because of the viewport i use).

The other changes look good.

comment:7 in reply to:  6 Changed 3 years ago by teor

Replying to cypherpunks:

Replying to RaBe:

We still need the fingerprint class to adjust the DDs margin, but I agree keeping it as a DD might be the better way :) Also I removed the trailing spaces.

https://github.com/RaphaelBergmann/atlas/commit/0a8aefbe54968e2e6aa302ad4dad4762c7ad77f6

I don't see why the margins of the fingerprint field should be altered. It requires custom code (which need to be maintained) and makes the look of the fingerprint inconsistent with other fields that use <pre>.

I agree - let's not change the margin.

Trying to get the fingerprint to use a single line is an unreachable goal because you don't know the sizes of all the viewports that are used. (For instance, on my end your branch still doesn't make the fingerprint use a single line because of the viewport i use).

I asked that it work for the default window size in Tor Browser, which seems reasonable.
(And it used to work, but was broken by recent changes.)

comment:8 Changed 3 years ago by RaBe

So... What about changing all margins just a bit? We would not need any extra classes. I don't care about the fingerprint wrapping on smaller screens, but I can't stand it wrapping just one or two chars in the medium view :D (Also I think changing from left to bottom margin even improves the detail page's visuals...)

https://github.com/RaphaelBergmann/atlas/commit/106e22f16fa8b197f0f408028bd7e4d59895096a

comment:9 in reply to:  8 Changed 3 years ago by cypherpunks

Status: needs_reviewneeds_revision

Replying to RaBe:

So... What about changing all margins just a bit? We would not need any extra classes.

I agree, this is a better solution.

I don't care about the fingerprint wrapping on smaller screens, but I can't stand it wrapping just one or two chars in the medium view :D

I get the frustration because it indeed looks weird to have a few characters dangling.

(Also I think changing from left to bottom margin even improves the detail page's visuals...)

I would write the rules differently so only specific bootstrap rules are overwritten and it would be a little bit more readable. See the inline patch for my suggestion.

diff --git a/css/style.css b/css/style.css
index 9fc862b..5e9000c 100644
--- a/css/style.css
+++ b/css/style.css
@@ -68,9 +68,10 @@ span.flags img {
 }
 
 dd {
-    margin:0 0 1.2em 0;
+    margin-left: 0;
+    margin-bottom: 1.2em;
 }
 
 pre {
-    padding:5px;
+    padding: 5px;
 }
Last edited 3 years ago by cypherpunks (previous) (diff)

comment:10 Changed 3 years ago by RaBe

Status: needs_revisionneeds_review

comment:11 Changed 3 years ago by cypherpunks

I'm okay with this. Leaving it to teor to set it to merge_ready if they are also okay with this.

comment:12 Changed 3 years ago by irl

Resolution: fixed
Status: needs_reviewclosed

Merged patch from comment:10. Works for me in Tor Browser with default viewport.

Thanks for your work on this. (:

comment:13 Changed 3 years ago by teor

I don't see any change in behaviour or appearance in Tor Browser 6.5 on macOS 10.12 with the default window size.

comment:14 in reply to:  13 Changed 3 years ago by teor

Replying to teor:

I don't see any change in behaviour or appearance in Tor Browser 6.5 on macOS 10.12 with the default window size.

Oops, I had the old CSS cached.

Note: See TracTickets for help on using tickets.