Paths

Table of Contentst

Differential D25619

Fix overflowing of AphrontSideNavFilterView on small screens and tidy up nav bar styles
AcceptedPublic
Actions

Authored by bekay on May 4 2024, 13:21.

Details

Reviewers

aklapper

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15809: Homepage will overflow on small screens when feed content is too long

Summary

Switch the two column layout of AphrontSideNavFilterView (used in pretty much most views) from table to flex display. The former CSS rendering could lead to overflowing of the entire page on small screens if very wide unbreaking content is displayed (example described here: T15809).

While I was at it I have found some unused code. The CSS file phabricator-nav-view.css consisted almost only of .phabricator-side-menu-home class selectors. This is an ancient class (there at least since 2013) not used anymore:

https://we.phorge.it/source/phorge/browse/master/src/applications/directory/controller/PhabricatorDirectoryController.php;919bd4a03499305093d8882952a9dd3dac1c55a9$169

So I have removed this CSS component and saved the few still used declarations to the main phui-basic-nav-view.css.

There was some unused code in AphrontSideNavFilterView.php too.

Fixes T15809

Test Plan

Look at this video to understand the problem:

F2178277

Go in a Task comment and mention a task with a very-long title. Have phd enabled to generate feeds. Visit homepage with feeds:

See that you cannot reproduce anymore.

Browse pages with the right nav bar with many devices and look if the layout looks fine and not broken.

Tested the homepage, scaling from mobile to desktop, using:

Firefox 124 64bit on GNU/Linux (snap)
Chromium 124 64bit on GNU/Linux (snap)
...

Tested other elements, scaling from mobile to desktop, still working correctly:

popup dialogs
right floating menu
top curtain
- notifications
- Conpherence chats
- unresolved setup issues
- search omnibox

Diff Detail

Repository

rP Phorge

Branch

master

Lint

Lint Passed

Unit

Test Failures

Build Status

Buildable 1299
Build 1299: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	128 ms	PhabricatorCelerityTestCase::testCelerityMaps Assertion failed, expected 'true' (at PhabricatorCelerityTestCase.php:32): When this test fails, it means the Celerity resource map is out of date. Run `bin/celerity map` to rebuild it. ACTUAL VALUE
	0 ms	PHUIListViewTestCase::testAppend 1 assertion passed.
	2 ms	PHUIListViewTestCase::testAppendAfter 2 assertions passed.
	0 ms	PHUIListViewTestCase::testAppendBefore 2 assertions passed.
	0 ms	PHUIListViewTestCase::testAppendLabel 2 assertions passed.
		View Full Test Results (1 Failed · 15 Passed)

Event Timeline

bekay created this revision.May 4 2024, 13:21

Owners added a reviewer: O1: Blessed Committers.May 4 2024, 13:21

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptMay 4 2024, 13:21

bekay requested review of this revision.May 4 2024, 13:21

Harbormaster completed remote builds in B1222: Diff 1934.May 4 2024, 13:21

bekay edited the summary of this revision. (Show Details)May 4 2024, 13:25

bekay added a task: T15809: Homepage will overflow on small screens when feed content is too long.

bekay edited the summary of this revision. (Show Details)May 4 2024, 13:28

screenies?

All described here with screenshot (and now video): T15809

But with my test you can just go to https://we.phorge.it with your mobile and see it for yourself.

Ah, yes thank you for the video. I should have clarified but was asking for screenies of the result after this change. The changes look reasonable but I’m not a regular css user

In D25619#17360, @speck wrote:

Ah, yes thank you for the video. I should have clarified but was asking for screenies of the result after this change. The changes look reasonable but I’m not a regular css user

The site looks the same of course just no overflowing 😋 long unbreaking elements are just cut off. Hopefully no big chance. But I think it should be checked out on some test enviromnents of busy reviewers.

valerio.bozzolan edited the summary of this revision. (Show Details)May 9 2024, 08:01

valerio.bozzolan edited the test plan for this revision. (Show Details)

Maybe "feature" but note the left sidebar, is not anymore connected to the end of the viewport:

Before	After

Fixes non stretching nav bar if content is absolute or does not fill entire page height

Harbormaster completed remote builds in B1299: Diff 2041.May 23 2024, 09:40

./bin/celerity map

Harbormaster completed remote builds in B1300: Diff 2042.May 23 2024, 09:43

In D25619#17560, @valerio.bozzolan wrote:

Maybe "feature" but note the left sidebar, is not anymore connected to the end of the viewport:

Before After

I have fixed this.

Hi bekay, thanks for looking into this, very appreciated!

Speaking as a reviewer, splitting into two patches would have been slightly easier (but speaking as a developer that's more fiddling and stacking is hard in arc so... your approach is understandable). :)

Removal of unused variables in src/view/layout/AphrontSideNavFilterView.php and basically webroot/rsrc/css/aphront/phabricator-nav-view.css makes sense - I verified via grep that variable aren't used within the PHP file and that the CSS file is not loaded/mentioned anywhere else.

Which leaves us with reviewing the changes in webroot/rsrc/css/application/base/standard-page-view.css and webroot/rsrc/css/phui/phui-basic-nav-view.css.
I tested on the frontpage and pages like http://phorge.localhost/diffusion/ with two varying widths: table-cell gets replaced by flex in the browser's developer tools; the rendered layout does stay the same.

Funny enough I do not see a change in behavior in Chromium in the Feed for mentioned tasks with a long title. Might be Chromium behaving differently here.

But I do see a welcome improvement on the frontpage:
Before:

D25619-frontpage-before.png (767×584 px, 95 KB)

After:

So all in all this looks good to me.
I'm going to accept personally for now.
If noone else who feels like a skilled CSS reviewer shows up for another review to also set Accept as O1: Blessed Committers, then I'll be happy to also accept as O1 in a week or such...

aklapper mentioned this in Z5: Phorge (flame).Jun 18 2024, 22:37

aklapper accepted this revision.Jun 21 2024, 13:28

This revision is now accepted and ready to land.Jun 21 2024, 13:28

aklapper retitled this revision from Fixes overflowing of AphrontSideNavFilterView on small screens and tidies up nav bar styles to Fix overflowing of AphrontSideNavFilterView on small screens and tidy up nav bar styles.Jun 21 2024, 13:28

@bekay: Would you like to merge this?