-
Notifications
You must be signed in to change notification settings - Fork 30
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
refactor(classifier): use native browser SVG scaling for drawing tools #6066
base: master
Are you sure you want to change the base?
refactor(classifier): use native browser SVG scaling for drawing tools #6066
Conversation
e2181da
to
6c6bd0e
Compare
6c6bd0e
to
8b56fe8
Compare
8b56fe8
to
538c44e
Compare
538c44e
to
6f70afa
Compare
6f70afa
to
4fa507f
Compare
2c6c8b5
to
3edd480
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments below, and just linking :focus-visible for documentation.
...lib-classifier/src/plugins/drawingTools/experimental/components/FreehandLine/FreehandLine.js
Outdated
Show resolved
Hide resolved
@@ -136,6 +137,7 @@ function FreehandLine({ active, mark, onFinish, scale }) { | |||
strokeWidth: GRAB_STROKE_WIDTH / scale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should GRAB_STROKE_WIDTH
still be divided by scale in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stroke width of each mark on a Correct-a-Cell project is much thinner on this branch than production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All stroke widths should be constant on this branch.
It’s been a while since I looked at the drawing code, but freehand lines might override the default stroke width, which is quite thick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visible stroke width for freehand lines is 1px on this branch, but maybe it should be 2px, or whatever the default stroke width is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed STROKE_WIDTH to 2px for freehand lines. I'm not sure why it was set to 1, when other tools use 2. Easy enough to change back, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some links for testing:
- Etch-A-Cell: https://localhost:8080/?project=h-spiers%2Fetch-a-cell&env=production&workflow=8921
- Correct-A-Cell: https://localhost:8080/?project=willcharlie/etch-a-cell-correct-a-cell&env=production&subject=93925884
When you compare those in production, you can see that Correct-A-Cell lines are thicker than Etch-A-Cell lines. They should be the same on this branch, and Etch-A-Cell freehand lines should be the same width here and in production (I think.)
I've been using Etch-A-Cell, in production, as the comparison for freehand line styling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Correct-A-Cell in production, the thickness of the freehand line tool even varies from one subject to another, because the subjects are different scales. I've opened #6167, but that should be fixed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please double check all drawing tools on this branch in comparison to production.
Here's a couple of different-sized Correct-A-Cell subjects. The 1200px subject will be the same stroke on this branch and in production. The 600px subject has a thinner stroke, but a thicker rendered line, in production, because the scale
multiplier is twice as large (1200/600) but the stroke width isn’t halved in the code. Both subjects should have the same thickness line on this branch, where image scale is handled by the browser and developers don’t have to remember to scale stroke width to subject image size in their code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's going to introduce a complication into the analysis of their beta classifications, since the line width wasn't constant from subject to subject. 😕
EDIT: even for the same subject, the line width will vary from volunteer to volunteer, according to the client width of the image in their browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goplayoutside3 the stroke width in production depends on both the subject image size (naturalWidth
) and the rendered image size in the browser (clientWidth
.) If you want me to match production styles, you’ll need to specify the subject image width and the subject viewer width, so I know what image scale to aim for. Roughly speaking, the production stroke width in SVG pixels is 2 * naturalWidth / clientWidth
for all the drawing tools except freehand lines.
Freehand lines use a scaling function that generates thicker lines for smaller subjects (or smaller browser windows.) See #6167 (comment)
All that means that if I want to match what you’re seeing, I need to know how much your browser is scaling the image by. It also means that individual volunteers for the Correct-A-Cell beta would have seen different line widths, depending on both the subject size and the size of their browser window.
strokeWidth={2} | ||
> | ||
<line x1={x1 + offsetX} y1={y1 + offsetY} x2={x2} y2={y2} /> | ||
<line x1={x1} y1={y1} x2={x2} y2={y2} strokeWidth={GRAB_STROKE_WIDTH / scale} strokeOpacity='0' /> | ||
<line x1={x1 + offsetX} y1={y1 + offsetY} x2={x2} y2={y2} vectorEffect={'non-scaling-stroke'} /> | ||
<line x1={x1} y1={y1} x2={x2} y2={y2} strokeWidth={GRAB_STROKE_WIDTH} strokeOpacity='0' vectorEffect={'non-scaling-stroke'} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line with GRAB_STROKE_WIDTH has opacity 0, so that’s fine.
It does look like the default stroke width is quite thick here. The visible line is the first line in the code (line 42.)
Line 43 is an invisible line that captures pointer events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the visible stroke width is 2px here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated transcription lines to explicitly set a stroke width of 2px.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike all the other drawing tools, transcription lines aren’t composed from Mark
components. That’s odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the other drawing tools, I’d recommend wrapping transcription lines in Mark
, so that they are composed from the same base settings as other tools. I haven’t done that here as it might be a breaking change for transcription workflows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the other drawing tools are wrapped in Mark
here:
Lines 78 to 117 in 68a3e2b
<Mark | |
key={mark.id} | |
isActive={isActive} | |
coords={mark.coords} | |
disabled={disabled} | |
dragStart={selectMark} | |
dragMove={moveMark} | |
dragEnd={endMoveMark} | |
label={`Mark ${index}`} | |
mark={mark} | |
onDelete={deleteMark} | |
onFinish={onFinish} | |
onSelect={selectMark} | |
pointerEvents={isActive ? 'painted' : pointerEvents} | |
scale={scale} | |
> | |
<MarkingComponent | |
active={isActive} | |
mark={mark} | |
onFinish={onFinish} | |
scale={scale} | |
played={played} | |
/> | |
{isActive && mark.tool.type !== 'freehandLine' && ( | |
<DeleteButton | |
label={`Delete ${tool.type}`} | |
mark={mark} | |
scale={scale} | |
onDelete={deleteMark} | |
onDeselect={deselectMark} | |
/> | |
)} | |
{isActive && mark.tool.type == 'freehandLine' && ( | |
<LineControls | |
mark={mark} | |
scale={scale} | |
onDelete={deleteMark} | |
/> | |
)} | |
</Mark> |
...lib-classifier/src/plugins/drawingTools/experimental/components/FreehandLine/FreehandLine.js
Outdated
Show resolved
Hide resolved
3edd480
to
76b3480
Compare
...lib-classifier/src/plugins/drawingTools/experimental/components/FreehandLine/FreehandLine.js
Outdated
Show resolved
Hide resolved
const dx = (self.rx + BUFFER) * Math.cos(theta) | ||
const dy = (self.ry + BUFFER) * Math.sin(theta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes a bug where zooming in or out moves the delete button on the ellipse tool for I Fancy Cats.
...ental/components/TranscriptionLine/components/TranscriptionLineMark/TranscriptionLineMark.js
Show resolved
Hide resolved
041afc4
to
30cda16
Compare
e64bca0
to
d9f65c3
Compare
const STROKE_WIDTH = 3 | ||
const SELECTED_STROKE_WIDTH = 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've based these numbers on a scale
of 0.67 but feel free to adjust them.
These set the styles for all drawing tools except the transcription line and freehand line, each of which has its own custom styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd prefer thicker drawing strokes for migrated projects, just adjust the numbers in the Mark
component.
By the way, front-end-monorepo/packages/lib-classifier/src/plugins/tasks/single/components/SingleChoiceTask.js Lines 12 to 15 in 68a3e2b
Lines 13 to 16 in 68a3e2b
|
2d08058
to
b22cf92
Compare
- remove manual scaling code from `strokeWidth` for all the SVG drawing marks. To be honest, it has never worked very well. - add `vector-effect="non-scaling-stroke"` to all the SVG drawing marks. This will make the displayed stroke width independent of the scaling of the SVG element, at all zoom levels. - style marks with `:focus-visible` so that the focus ring is only shown for keyboard interactions.
- Use SVG presentation attributes for line styles. - Specify a stroke width of 2px.
…components/FreehandLine/FreehandLine.js Co-authored-by: Delilah C. <[email protected]>
- remove redundant `scale` prop. - adjust stroke width.
b22cf92
to
3cecfd5
Compare
strokeWidth
for all the SVG drawing marks. To be honest, it has never worked very well. When I wrote it, I didn't know that you can disable stroke scaling programmatically in SVG.vector-effect="non-scaling-stroke"
to all the SVG drawing marks. This will make the displayed stroke width independent of the scaling of the SVG<image>
, at all zoom levels.style marks with:focus-visible
so that the focus ring is only shown for keyboard interactions.Screen.Recording.2024-04-28.at.12.06.37.mov
Package
Linked issues
How to Review
This staging workflow has a mixture of drawing tools:
https://localhost:8080/?project=908&workflow=3370
A couple of workflows that use freehand lines:
Wildwatch Burrowing Owl uses the rectangle drawing tool:
You can also try out the drawing step in the I Fancy Cats workflow or try out a transcription task, with annotations from Caesar:
https://localhost:8080/?project=mainehistory/beyond-borders-transcribing-historic-maine-land-documents&env=production
You can test the focus styling by tabbing through the page, to check that drawn marks have focus styling when focused (WCAG Success Criterion 2.4.7: focus visible.) When a mark has keyboard focus, Enter will open any associated subtask popup, which should also be fully keyboard accessible. Backspace will delete the focused mark (WCAG Success Criterion 2.1.1: Keyboard.)
Screenshots
Each of these screenshots is taken from a production build of the project app, running in Firefox 128. Subject scale is
clientWidth / naturalWidth
for the subject image. Scaling shouldn't make a difference on this branch, but does affect the rendered stroke width in production.Wildwatch Burrowing Owl with an active (6px stroke) and inactive (3px stroke) rectangle (subject scale: 0.44):
https://local.zooniverse.org:3000/projects/sandiegozooglobal/wildwatch-burrowing-owl/classify/workflow/26173
Correct-A-Cell with a 1200px subject (scale: 0.74):
https://local.zooniverse.org:3000/projects/willcharlie/etch-a-cell-correct-a-cell/classify/workflow/24998/subject/93925784
Correct-A-Cell with a 600px subject (scale: 1.48):
https://local.zooniverse.org:3000/projects/willcharlie/etch-a-cell-correct-a-cell/classify/workflow/24998/subject/93925884
Checklist
PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.
General
yarn panic && yarn bootstrap
ordocker-compose up --build
and FEM works as expectedGeneral UX
Example Staging Project: i-fancy-cats
Refactoring