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.

Sign up for GitHub

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

Jump to bottom

Code Runner: The full-screen and exit buttons lack keyboard tab support #773786 #209

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Code Runner: The full-screen and exit buttons lack keyboard tab support #773786 #209

merged 1 commit into from
Apr 4, 2024

Conversation

danghieu1407
Copy link
Contributor

Hi @timhunt, @trampgeek,
I have removed the tab index for using the "Tab" key to the button fullscreen and added the styling for buttons fullscreen and exit fullscreen button.

This modification is compatible with Firefox, Edge, Safari, and Chrome. Here's a screenshot of the implementation:
image

The bottom of the editor is too small as in the image below. The bottom outline of the button when focusing will not display fully like this:
The bottom outline:
image
The bottom editor:
image
I have used CSS to fix this in this pull request. This issue happens because of the position-relative of the wrapper. But we must use the position-relative because it will help the scratchpad UI display two fullscreen buttons.

Could I expand the height bottom of the editor by 1 to 2 px in this code 541. This adjustment will address the bottom outline issues across different browsers and reduce the need for extensive CSS when focusing on buttons.

What do you think @timhunt, @trampgeek? If you disagree with expanding the bottom of the editor and prefer fixing it using CSS, please let me know. The code is ready for review (I have fixed the bottom outline in this PR, and test in Chrome, Edge, Firefox, and Safari).
I hope this clarifies the situation for you.
Many thanks.

@trampgeek
Copy link
Owner

Well done on finding a pure CSS fix. I was thinking of a more complex solution that involved using JavaScript to wrap the wrapper.

I am however a bit sad to see the 2px black rectangle around the full-screen image. I particularly liked how discreet the previous icon was. Once you knew about it, it was trivial to find it but it wasn't in your face. Since I don't think most users will want this functionality, at least in our environment, I'd prefer to keep it unobtrusive myself. And that would presumably mean you didn't need to expand the bottom of the editor, although that doesn't particularly worry me.

How would you feel about running without the border for a while to get a feel for how much confusion the lack of a border causes?

However, since OU is driving this, I don't wish to push my arguments too hard. How much feedback have you gathered on the appearance of the current version? I assume Tim is in favour of the louder icon?

@danghieu1407
Copy link
Contributor Author

Hi @trampgeek, Thanks for your feedback.
Sorry about making you confused. I have not changed the icon before. The rectangle border is only displayed when we use the "TAB" key.
We can customize the border of the icon by using this selector 285.

I want to increase the height of the bottom editor from 1 to 2 pixels. This change is necessary because, in certain browsers, such as Firefox, the resize icon near the button fullscreen is bigger than in another browser.
image

Additionally, on the question editing page, I aim to ensure that the border (the black rectangle) is fully displayed when a user utilizes the "TAB" key to navigate to the fullscreen button. Consequently, I have adjusted its position slightly upward, as illustrated in the image below:
image
Increasing the height of the bottom editor from 1 to 2 pixels can make the fullscreen button center.

I hope this makes sense to you. Thanks you @trampgeek.

@trampgeek
Copy link
Owner

Oh, I'm sorry danghieu. I should have studied your code more closely to spot that the border was only there when selected by a TAB.

I'm totally happy for you to increase the GUTTER variable in the UI wrapper by 1 or 2 pixels. Do you want to do that before I merge? I'm happy to merge right now if you prefer.

@danghieu1407
Copy link
Contributor Author

Hi @trampgeek,
I'll update this pull request and send it over for your review once I've completed the changes.
Many thanks.

@trampgeek trampgeek merged commit 0546df9 into trampgeek:master Apr 4, 2024
4 of 5 checks passed
@trampgeek
Copy link
Owner

Many thanks for all the work on this @danghieu1407. I dont understand the CI fail case - it looks spurious, and I don't see how it can possibly be due to your changes. So I've merged and we'll see what happens on the next CI run.

@danghieu1407
Copy link
Contributor Author

danghieu1407 commented Apr 4, 2024 •
edited
Loading

Hi @trampgeek,
I saw the previous CI failed before as shown in the image below:
image
This is unrelated to the fix I am doing.
Thank you very much, @trampgeek.

@trampgeek
Copy link
Owner

True, and that one looked spurious, too. It was a different error: "ERR_CONNECTION_REFUSED" on trying to contact Jobe. Which makes no sense. The Master branch hasn't changed since your first pull request, which didn't throw up any CI errors.

Perhaps it's the fact that the php 8.2 case - the one that has failed both times - is using moodle-branch 'master', whereas the other tests are all with moodle-branches tagged STABLE.

@danghieu1407
Copy link
Contributor Author

Hi @trampgeek,
I have a look for this CI fail. To demonstrate my pull request is not a reason for the CI failure.
This is an image for running CI:
image

First, I suspect my change of the JavaScript file (meaning the change of the GUTTER increasing to 2px), So I have reset the GUTTER to default (t.GUTTER = 14) and add const hAdjusted = h - this.GUTTER - 2; in 541 . Then I push it to my temporary branch. Then the CI notify failed (test cli)

Second, I have reverted all changes in the JS file (meaning I just pushed the screenmode_button.mustache, and style.css files). Then I push it to my temporary branch. Then the CI notify failed (test cli1)

Third, I have reverted all changes including the style.css file(meaning I just pushed the screenmode_button.mustache) Then I pushed it to my temporary branch. Then the CI notify failed (test cli2)

This is my CI result link: CI result

Finally, I ran that fail at my local and the result got passed.
image

I think my change is not a reason for the CI failure.
I hope this makes sense to you.
Many thanks

@trampgeek
Copy link
Owner

trampgeek commented Apr 4, 2024 via email

@danghieu1407
Copy link
Contributor Author

Hi @trampgeek,
Sorry about making you think like that. I'm just trying to fix the CI fail.
I have pulled the latest code there is a failure when we back up a code runner. Same the Ci Fail as the image below:
image
image

The root cause of the failure is a new change in the Moodle master.
Many thanks, @trampgeek.

@danghieu1407
Copy link
Contributor Author

Hi @trampgeek, I have found the root cause and fix it in Pull request Behat fix Pull Request.

trampgeek pushed a commit that referenced this pull request Apr 14, 2024
@danghieu1407 @trampgeek
Code Runner: The full-screen and exit buttons lack keyboard tab suppo…
ceeeb77 
…rt #773786 (#209)

Co-authored-by: DangHieu1407 <hnd34@open.ac.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danghieu1407 @trampgeek