Skip to content

Commit

Permalink
Hack mobile CSS together
Browse files Browse the repository at this point in the history 
- Still requires patch on mCaptcha's side to provide good iframe
settings & good CSS on widget HTML.
  • Loading branch information
Gusted committed Jul 26, 2022
1 parent 08790c5 commit 0789b33
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 3 deletions.
6 changes: 3 additions & 3 deletions templates/user/auth/signup_inner.tmpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@
</div>
{{end}}
{{if and .EnableCaptcha (eq .CaptchaType "mcaptcha")}}
<div class="inline field required df ac">
<label></label>
<div class="border-secondary" id="mcaptcha__widget-container" style="width: 304px; height: 78px"></div>
<div class="inline field required df ac db-small">
<label>{{.locale.Tr "captcha"}}</label>
<div class="border-secondary w-100-small" id="mcaptcha__widget-container" style="width: 50%; height: 5em"></div>
<div class="m-captcha" data-sitekey="{{ .McaptchaSitekey }}" data-instance-url="{{ .McaptchaURL }}"></div>
</div>
{{end}}
Expand Down
5 changes: 5 additions & 0 deletions web_src/less/helpers.less
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,8 @@
.py-3 { padding-top: .5rem !important; padding-bottom: .5rem !important; }
.py-4 { padding-top: 1rem !important; padding-bottom: 1rem !important; }
.py-5 { padding-top: 2rem !important; padding-bottom: 2rem !important; }

@media @mediaSm {
.db-small { display: block !important; }
.w-100-small { width: 100% !important; }
}

9 comments on commit 0789b33

@fsologureng
Copy link
Contributor

@fsologureng fsologureng commented on 0789b33 Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, in my opinion that label tag is not usable because it doesn't have the for attribute nor does it contain an input, so it could affect accessibility. A stylized title would be fine but I think it is not necessary if the tag is around or for the token input in the widget. However, a hidden input type for the token would be better if there is a title for all the captcha stuff and not an orphaned label.
The widget is already in production so my suggestion here and there might break the presentation in those places, and further complicate the situation, but I think it would improve its usability.

@Gusted
Copy link
Contributor

@Gusted Gusted commented on 0789b33 Jul 27, 2022

Choose a reason for hiding this comment

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

Interesting, didn't thought about that. However due to the styling that's used by Gitea, an <label> needs to be placed there and on Mobile it looked nicer with actual "Captcha" as content.

It's not a big deal if it's removed, however what would be the best way to still have that element there but not affect accessibility? An aria-hidden=true attribute perhaps?

@fsologureng
Copy link
Contributor

Choose a reason for hiding this comment

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

The Gitea semantic is ok for the login and password because they have their for attribute pointing to their <input>. But in this case, the interactive input (checkbox) is in the iframe, out of this form, so the label and the token input doesn't have interactive meaning in the form and it will be ok if both were hidden. But, if you add the aria-hidden attribute to that label, where the accessibility reader read the Captcha word? They only read the mutable label of the checkbox (if they can).That's what the reason to thinking in a title more than a label tag (always associated with an input). But yes, it must be a stylized title that doesn't break the whole composition. One can think in a legend tag, but this form doesn't have fieldset containers and that change would be more complicated.

@Gusted
Copy link
Contributor

@Gusted Gusted commented on 0789b33 Jul 28, 2022

Choose a reason for hiding this comment

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

I might misunderstand you here, so correct me if I'm wrong. If I read your comment correctly, when the code is like:

<label aria-hidden="true">{{.locale.Tr "captcha"}}</label>
<div title="{{.locale.Tr "captcha"}}" class="border-secondary w-100-small" id="mcaptcha__widget-container" style="width: 50%; height: 5em"></div>

It will be more correct for a screen reader and be more understandable towards the user about that this is a captcha?

@fsologureng
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, when I wrote title I was thinking of a <h5> or a <span> with a css style that provides identical behaviour to <label>, not a title attribute. Your idea looks good but I just doubt whether the focus goes to the container div before the checkbox, which should be ideal, so I'm not entirely sure. You have to think that the reader is iterating through the elements giving focus each one at a time and playing sound the associated text, so if the focus goes first to the checkbox rather than the container div, the experience can be weird.
But maybe sufficient at this stage.

On the other hand, I'm trying to find out why this html is not proposed in the mcaptcha module or in a code in the widget but in this template. I have seen that the empty tag is not needed in recaptcha or hcaptcha, can you please explain?

@Gusted
Copy link
Contributor

@Gusted Gusted commented on 0789b33 Jul 28, 2022

Choose a reason for hiding this comment

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

Sorry, when I wrote title I was thinking of a <h5> or a <span> with a css style that provides identical behaviour to <label>

Yeah that's a bit of a problem. Gitea's current layout relies on having a <label> there. It should be fine if <span> is also considered "okay" for the current layout.

On the other hand, I'm trying to find out why this html is not proposed in the mcaptcha module or in a code in the widget but in this template. I have seen that the empty tag is not needed in recaptcha or hcaptcha, can you please explain?

They are propagated by external code and they actually should have a label with captcha IMO, but not the scope of this PR. So this is the more correct version.
For the mCaptcha code we just provide a base container(and should handle dimensions, styling, accessibility etc.) and the code will just attach mCaptcha's widget into a iframe onto that container.

@fsologureng
Copy link
Contributor

Choose a reason for hiding this comment

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

They are propagated by external code and they actually should have a label with captcha IMO, but not the scope of this PR. So this is the more correct version. For the mCaptcha code we just provide a base container(and should handle dimensions, styling, accessibility etc.) and the code will just attach mCaptcha's widget into a iframe onto that container.

Understood. Thank you.

@Gusted
Copy link
Contributor

@Gusted Gusted commented on 0789b33 Jul 29, 2022

Choose a reason for hiding this comment

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

@fsologureng FYI, f4e417b replaced the label with span. So it's now mCaptcha's job to provide a widget which is accessible(which is easier to do).

@fsologureng
Copy link
Contributor

Choose a reason for hiding this comment

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

Very good :)

Please sign in to comment.