Remove WebApp/PWA-specific minimum content width [chromium/src : main]
ilsubyeega (Gerrit)
Message from ilsubyeega![]( "Open in Gerrit")
Set Ready For Review
Related details
- Dana Fried
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted Code-Review+1![]( "Open in Gerrit")
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Related details
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dana Fried
| Code-Review | -1 |
1 comment
The minimum content width is provided to ensure that PWAs receive enough space to render. This is a guarantee PWA authors have been able to rely on for a long time (while the logic has moved, the values have been in place for years).
Changing this has the potential to break certain PWAs for some users/authors.
In addition, none of the attached bugs are related to the computation you are removing here. The value here is the minimum width of the _content area_ - the minimum width of the toolbar (if present) and other window features is computed separately and still have the same behaviors.
Also this is the code for PWAs only, whereas those bugs often referred to tabbed browsers.
I'm not sure what you're trying to do here. If you have a specific use case, it would be good to file a bug that is less than 5 years old and we can talk to our PWA and enterprise people about whether we should still be enforcing this minimum width.
Sorry, and thank you.
Related details
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ilsubyeega (Gerrit)
ilsubyeega added 1 comment![]( "Open in Gerrit")
The minimum content width is provided to ensure that PWAs receive enough space to render. This is a guarantee PWA authors have been able to rely on for a long time (while the logic has moved, the values have been in place for years).
Changing this has the potential to break certain PWAs for some users/authors.
In addition, none of the attached bugs are related to the computation you are removing here. The value here is the minimum width of the _content area_ - the minimum width of the toolbar (if present) and other window features is computed separately and still have the same behaviors.
Also this is the code for PWAs only, whereas those bugs often referred to tabbed browsers.
I'm not sure what you're trying to do here. If you have a specific use case, it would be good to file a bug that is less than 5 years old and we can talk to our PWA and enterprise people about whether we should still be enforcing this minimum width.
Sorry, and thank you.
[...] the values have been in place for years).
While this was landed in Feb this year[1], the old landing at 2019 seems to be intended/scoped in Tabbed-mode[2]. I do not have access/permission the bug #486792513, as "Access is denied to this issue", Please LMK If it's available or may get summary of that.
none of the attached bugs are related to the computation you are removing here.
I might agree as computation, but I believe its based on context(each of bugs: umbrella bug, the bug title, the bug that landed this bug again).
[...] is computed separately and still have the same behaviors.
I did verify by running binary own and can confirm this matched my intend.
[...] we can talk to our PWA and enterprise people about whether we should still be enforcing this minimum width.
I think it should. This may be good issue: https://issues.chromium.org/issues/507898379 "you cant even fit 3 chrome window on the screen at the same time"
I would like to ask visit the gist which contains image and videos, which makes me this review: https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76
Thanks for your review as I used gerrit first time, so do much confusing here.
[1]: https://chromium-review.googlesource.com/c/chromium/src/+/7600512
[2]: https://source.chromium.org/chromium/chromium/src/+/580d6900dc7fc0d7f5b10d2375585e075f79329a
Related details
- Dana Fried
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
ilsubyeegaThe minimum content width is provided to ensure that PWAs receive enough space to render. This is a guarantee PWA authors have been able to rely on for a long time (while the logic has moved, the values have been in place for years).
Changing this has the potential to break certain PWAs for some users/authors.
In addition, none of the attached bugs are related to the computation you are removing here. The value here is the minimum width of the _content area_ - the minimum width of the toolbar (if present) and other window features is computed separately and still have the same behaviors.
Also this is the code for PWAs only, whereas those bugs often referred to tabbed browsers.
I'm not sure what you're trying to do here. If you have a specific use case, it would be good to file a bug that is less than 5 years old and we can talk to our PWA and enterprise people about whether we should still be enforcing this minimum width.
Sorry, and thank you.
[...] the values have been in place for years).
While this was landed in Feb this year[1], the old landing at 2019 seems to be intended/scoped in Tabbed-mode[2]. I do not have access/permission the bug #486792513, as "Access is denied to this issue", Please LMK If it's available or may get summary of that.
none of the attached bugs are related to the computation you are removing here.
I might agree as computation, but I believe its based on context(each of bugs: umbrella bug, the bug title, the bug that landed this bug again).
[...] is computed separately and still have the same behaviors.
I did verify by running binary own and can confirm this matched my intend.
[...] we can talk to our PWA and enterprise people about whether we should still be enforcing this minimum width.
I think it should. This may be good issue: https://issues.chromium.org/issues/507898379 "you cant even fit 3 chrome window on the screen at the same time"
I would like to ask visit the gist which contains image and videos, which makes me this review: https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76
Thanks for your review as I used gerrit first time, so do much confusing here.
[1]: https://chromium-review.googlesource.com/c/chromium/src/+/7600512
[2]: https://source.chromium.org/chromium/chromium/src/+/580d6900dc7fc0d7f5b10d2375585e075f79329a
I'm going to be out next week so I'm trying to find someone on my team who can loop in the necessary stakeholders. Until we've got their go-ahead I'm afraid the CL cannot proceed.
Again sorry for the inconvenience/delay.
Related details
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ilsubyeega (Gerrit)
ilsubyeega added 1 comment![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ilsubyeega (Gerrit)
Just for sure, this may removes tabbed PWA behavior, which i think available at chromeos, other platforms are flagged-behind. I might just guard that into chromeos or flag only condition if concerned. (I tested tabbed PWA at linux and didnt work well so)
Related details
- Dana Fried
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ilsubyeega (Gerrit)
good morning. bumping out after a week, would appreciated to check, ty!
Related details
- Dana Fried
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
I've reached out to a contact on the PWA team; we're getting this routed to the correct people. Thank you for your patience.
Related details
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted Code-Review+0![]( "Open in Gerrit")
- Dibyajyoti Pal
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Code-Review | -1 |
1 comment
@dmu...@chromium.org interested in what you think here.
Based on my understanding of the intent provided on [Github](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76), I don't think this is the correct behavior. The minimum width of the PWA window takes into account various security specific behavior of PWA windows, like enough width to show the origin text and animate it. From the intent, it seems like the window is too short for the origin to show up, so these changes are not correct.
At the very least, if you want the PWA windows to be shorter and not tied to the browser window's size, we should have enough space to show the origin text, the app's title and other icons on the toolbar to prevent users from being spoofed.
https://g-issues.chromium.org/issues/40614379#comment30 also says that the window size of PWAs were brought down to 150px, can you verify if that helps your use-case?
I could be wrong here though, in which case, happy to take back my -1, but this seems incorrect.
Related details
- Daniel Murphy
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy added 2 comments![]( "Open in Gerrit")
Removing this minimum width is desirable to allow smaller utility PWA windows (as requested in b/41408600). However, doing so risks regressing b/486792513 (PWA menu dropdown clipping on Linux X11) and b/40614379 (Find-in-page clipping).
The CL description mentions mitigation via `TabModalUsesDesktopWidget` feature flag. However:
1. This flag is disabled by default (`base::FEATURE_DISABLED_BY_DEFAULT`). If we rely on it, we might need to enable it by default first, or at least for PWAs.
2. It's unclear if `TabModalUsesDesktopWidget` actually affects the PWA app menu dropdown or Find-in-page bubble, as they might not be classified as "tab modal dialogs" managed by `TabDialogManager`. Find-in-page uses `TYPE_CONTROL` widget which is constrained by host.
Have we verified if these elements get clipped when the PWA window is resized to the new minimum width (which can be < 500px)?
const gfx::Size min_size = helper()->browser_view()->GetMinimumSize();We should also verify that tabbed PWAs do not suffer from overlapping elements (new tab button, extension button, origin text) at the new minimum width, which was the issue in b/40708657.
Could we add a test case or verify manually that the layout handles very narrow widths gracefully without overlaps?
Related details
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy added 1 comment![]( "Open in Gerrit")
@dmu...@chromium.org interested in what you think here.
Based on my understanding of the intent provided on [Github](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76), I don't think this is the correct behavior. The minimum width of the PWA window takes into account various security specific behavior of PWA windows, like enough width to show the origin text and animate it. From the intent, it seems like the window is too short for the origin to show up, so these changes are not correct.
At the very least, if you want the PWA windows to be shorter and not tied to the browser window's size, we should have enough space to show the origin text, the app's title and other icons on the toolbar to prevent users from being spoofed.
https://g-issues.chromium.org/issues/40614379#comment30 also says that the window size of PWAs were brought down to 150px, can you verify if that helps your use-case?
I could be wrong here though, in which case, happy to take back my -1, but this seems incorrect.
I agree - we need to make sure this doesn't cause issues with things like the origin text clipping. I think we scroll it - but yes. If there is a test we can have here around making sure that minimum content width includes also the toolbar stuff, then great. but I agree - there is a reason we did this.
ilsubyeega (Gerrit)
ilsubyeega added 3 comments![]( "Open in Gerrit")
Daniel Murphy@dmu...@chromium.org interested in what you think here.
Based on my understanding of the intent provided on [Github](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76), I don't think this is the correct behavior. The minimum width of the PWA window takes into account various security specific behavior of PWA windows, like enough width to show the origin text and animate it. From the intent, it seems like the window is too short for the origin to show up, so these changes are not correct.
At the very least, if you want the PWA windows to be shorter and not tied to the browser window's size, we should have enough space to show the origin text, the app's title and other icons on the toolbar to prevent users from being spoofed.
https://g-issues.chromium.org/issues/40614379#comment30 also says that the window size of PWAs were brought down to 150px, can you verify if that helps your use-case?
I could be wrong here though, in which case, happy to take back my -1, but this seems incorrect.
I agree - we need to make sure this doesn't cause issues with things like the origin text clipping. I think we scroll it - but yes. If there is a test we can have here around making sure that minimum content width includes also the toolbar stuff, then great. but I agree - there is a reason we did this.
we should have enough space to show the origin text, the app's title and other icons on the toolbar to prevent users from being spoofed.
Would showing only icons, dropdown menu, window controls(- ㅁ X) work? Showing origin text(titles?) and app title would not able to work as it contains window controls by default(or maybe we can hide that and only show when hovered, but this should be addressed with separate issue)
https://g-issues.chromium.org/issues/40614379#comment30 also says that the window size of PWAs were brought down to 150px, can you verify if that helps your use-case?
This is re-landing of that idea but it contained multiple issues, find-in-page do not work, buttons overlapped etc. At that time it was extremely narrower(86px) than this patch does.
[image of chromium-145](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269339#gistcomment-6269339)
Removing this minimum width is desirable to allow smaller utility PWA windows (as requested in b/41408600). However, doing so risks regressing b/486792513 (PWA menu dropdown clipping on Linux X11) and b/40614379 (Find-in-page clipping).
The CL description mentions mitigation via `TabModalUsesDesktopWidget` feature flag. However:
1. This flag is disabled by default (`base::FEATURE_DISABLED_BY_DEFAULT`). If we rely on it, we might need to enable it by default first, or at least for PWAs.
2. It's unclear if `TabModalUsesDesktopWidget` actually affects the PWA app menu dropdown or Find-in-page bubble, as they might not be classified as "tab modal dialogs" managed by `TabDialogManager`. Find-in-page uses `TYPE_CONTROL` widget which is constrained by host.
Have we verified if these elements get clipped when the PWA window is resized to the new minimum width (which can be < 500px)?
You're right.
I've checked both X11 and wayland ozone, "find in page" UI do regress(breaks, mentioned in github gist intent), while app menu dropdown(the three-dot menu in the top bar) shows in separate surface.
The `TabModalUsesDesktopWidget` mention was for breaking other UIs, See [this gist comment](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269320#gistcomment-6269320), I attached the image for example. (and just found that closing popups via `Esc` key does not work when enabled)
Also I'm interested for looking b/486792513, I do not have perms to read, Can I get brief info of that bug?
const gfx::Size min_size = helper()->browser_view()->GetMinimumSize();We should also verify that tabbed PWAs do not suffer from overlapping elements (new tab button, extension button, origin text) at the new minimum width, which was the issue in b/40708657.
Could we add a test case or verify manually that the layout handles very narrow widths gracefully without overlaps?
I think I don't have any working tabbed PWA from my side at this time(Linux wayland), so I might need to get direction to figure out.
Related details
- Daniel Murphy
- Dibyajyoti Pal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ilsubyeega (Gerrit)
ilsubyeega added 1 comment![]( "Open in Gerrit")
It seems I need to set this to Active to send notifications/attentions to the reviewer. I apologize for the inconvenience/spam.
Daniel Murphy (Gerrit)
Daniel Murphy added 1 comment![]( "Open in Gerrit")
ilsubyeegaRemoving this minimum width is desirable to allow smaller utility PWA windows (as requested in b/41408600). However, doing so risks regressing b/486792513 (PWA menu dropdown clipping on Linux X11) and b/40614379 (Find-in-page clipping).
The CL description mentions mitigation via `TabModalUsesDesktopWidget` feature flag. However:
1. This flag is disabled by default (`base::FEATURE_DISABLED_BY_DEFAULT`). If we rely on it, we might need to enable it by default first, or at least for PWAs.
2. It's unclear if `TabModalUsesDesktopWidget` actually affects the PWA app menu dropdown or Find-in-page bubble, as they might not be classified as "tab modal dialogs" managed by `TabDialogManager`. Find-in-page uses `TYPE_CONTROL` widget which is constrained by host.
Have we verified if these elements get clipped when the PWA window is resized to the new minimum width (which can be < 500px)?
You're right.
I've checked both X11 and wayland ozone, "find in page" UI do regress(breaks, mentioned in github gist intent), while app menu dropdown(the three-dot menu in the top bar) shows in separate surface.
The `TabModalUsesDesktopWidget` mention was for breaking other UIs, See [this gist comment](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269320#gistcomment-6269320), I attached the image for example. (and just found that closing popups via `Esc` key does not work when enabled)
Also I'm interested for looking b/486792513, I do not have perms to read, Can I get brief info of that bug?
for that bug, if the window was too narrow, then the 3-dot menu list would clip, where the left side of the menu would just not render at all.
Related details
- Dibyajyoti Pal
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
Daniel Murphy@dmu...@chromium.org interested in what you think here.
Based on my understanding of the intent provided on [Github](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76), I don't think this is the correct behavior. The minimum width of the PWA window takes into account various security specific behavior of PWA windows, like enough width to show the origin text and animate it. From the intent, it seems like the window is too short for the origin to show up, so these changes are not correct.
At the very least, if you want the PWA windows to be shorter and not tied to the browser window's size, we should have enough space to show the origin text, the app's title and other icons on the toolbar to prevent users from being spoofed.
https://g-issues.chromium.org/issues/40614379#comment30 also says that the window size of PWAs were brought down to 150px, can you verify if that helps your use-case?
I could be wrong here though, in which case, happy to take back my -1, but this seems incorrect.
ilsubyeegaI agree - we need to make sure this doesn't cause issues with things like the origin text clipping. I think we scroll it - but yes. If there is a test we can have here around making sure that minimum content width includes also the toolbar stuff, then great. but I agree - there is a reason we did this.
we should have enough space to show the origin text, the app's title and other icons on the toolbar to prevent users from being spoofed.
Would showing only icons, dropdown menu, window controls(- ㅁ X) work? Showing origin text(titles?) and app title would not able to work as it contains window controls by default(or maybe we can hide that and only show when hovered, but this should be addressed with separate issue)
https://g-issues.chromium.org/issues/40614379#comment30 also says that the window size of PWAs were brought down to 150px, can you verify if that helps your use-case?
This is re-landing of that idea but it contained multiple issues, find-in-page do not work, buttons overlapped etc. At that time it was extremely narrower(86px) than this patch does.
[image of chromium-145](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269339#gistcomment-6269339)
Would showing only icons, dropdown menu, window controls(- ㅁ X) work? Showing origin text(titles?) and app title would not able to work as it contains window controls by default(or maybe we can hide that and only show when hovered, but this should be addressed with separate issue)
Sadly no. The app title and the origin text HAS to show up on the PWA window afaik, because those are necessary anti-spoofing measures for the user.
Related details
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
const gfx::Size min_size = helper()->browser_view()->GetMinimumSize();ilsubyeegaWe should also verify that tabbed PWAs do not suffer from overlapping elements (new tab button, extension button, origin text) at the new minimum width, which was the issue in b/40708657.
Could we add a test case or verify manually that the layout handles very narrow widths gracefully without overlaps?
I think I don't have any working tabbed PWA from my side at this time(Linux wayland), so I might need to get direction to figure out.
Will you be able to test out tabbed mode with some a test site like https://googlechrome.github.io/samples/pwa-testing/flossy-neon-sugar/? You might need to enable the 3 flags from chrome://flags for verifying tabbed mode:
- chrome://flags/#enable-desktop-pwas-tab-strip
- chrome://flags/#enable-desktop-pwas-tab-strip-settings
- chrome://flags/#enable-desktop-pwas-tab-strip-customizations
Either way, the UX requirement for anti-spoofing still remains.
ilsubyeega (Gerrit)
ilsubyeega added 3 comments![]( "Open in Gerrit")
Daniel Murphy@dmu...@chromium.org interested in what you think here.
Based on my understanding of the intent provided on [Github](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76), I don't think this is the correct behavior. The minimum width of the PWA window takes into account various security specific behavior of PWA windows, like enough width to show the origin text and animate it. From the intent, it seems like the window is too short for the origin to show up, so these changes are not correct.
At the very least, if you want the PWA windows to be shorter and not tied to the browser window's size, we should have enough space to show the origin text, the app's title and other icons on the toolbar to prevent users from being spoofed.
https://g-issues.chromium.org/issues/40614379#comment30 also says that the window size of PWAs were brought down to 150px, can you verify if that helps your use-case?
I could be wrong here though, in which case, happy to take back my -1, but this seems incorrect.
ilsubyeegaI agree - we need to make sure this doesn't cause issues with things like the origin text clipping. I think we scroll it - but yes. If there is a test we can have here around making sure that minimum content width includes also the toolbar stuff, then great. but I agree - there is a reason we did this.
Dibyajyoti Palwe should have enough space to show the origin text, the app's title and other icons on the toolbar to prevent users from being spoofed.
Would showing only icons, dropdown menu, window controls(- ㅁ X) work? Showing origin text(titles?) and app title would not able to work as it contains window controls by default(or maybe we can hide that and only show when hovered, but this should be addressed with separate issue)
https://g-issues.chromium.org/issues/40614379#comment30 also says that the window size of PWAs were brought down to 150px, can you verify if that helps your use-case?
This is re-landing of that idea but it contained multiple issues, find-in-page do not work, buttons overlapped etc. At that time it was extremely narrower(86px) than this patch does.
[image of chromium-145](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269339#gistcomment-6269339)
Would showing only icons, dropdown menu, window controls(- ㅁ X) work? Showing origin text(titles?) and app title would not able to work as it contains window controls by default(or maybe we can hide that and only show when hovered, but this should be addressed with separate issue)
Sadly no. The app title and the origin text HAS to show up on the PWA window afaik, because those are necessary anti-spoofing measures for the user.
I have question the sample image of the tabbed PWA demo does not enforce any title, URLs. Was it only used for testing purpose? May I get rechecked for this purpose? This patch directly challenges to anti-spoofing policy.
Or we can have workaround for retriving title and urls via hover to non-tabbed PWA like tabbed PWA current do. [image](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269370#gistcomment-6269370)
ilsubyeegaRemoving this minimum width is desirable to allow smaller utility PWA windows (as requested in b/41408600). However, doing so risks regressing b/486792513 (PWA menu dropdown clipping on Linux X11) and b/40614379 (Find-in-page clipping).
The CL description mentions mitigation via `TabModalUsesDesktopWidget` feature flag. However:
1. This flag is disabled by default (`base::FEATURE_DISABLED_BY_DEFAULT`). If we rely on it, we might need to enable it by default first, or at least for PWAs.
2. It's unclear if `TabModalUsesDesktopWidget` actually affects the PWA app menu dropdown or Find-in-page bubble, as they might not be classified as "tab modal dialogs" managed by `TabDialogManager`. Find-in-page uses `TYPE_CONTROL` widget which is constrained by host.
Have we verified if these elements get clipped when the PWA window is resized to the new minimum width (which can be < 500px)?
Daniel MurphyYou're right.
I've checked both X11 and wayland ozone, "find in page" UI do regress(breaks, mentioned in github gist intent), while app menu dropdown(the three-dot menu in the top bar) shows in separate surface.
The `TabModalUsesDesktopWidget` mention was for breaking other UIs, See [this gist comment](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269320#gistcomment-6269320), I attached the image for example. (and just found that closing popups via `Esc` key does not work when enabled)
Also I'm interested for looking b/486792513, I do not have perms to read, Can I get brief info of that bug?
for that bug, if the window was too narrow, then the 3-dot menu list would clip, where the left side of the menu would just not render at all.
Was that issue intended like [this image](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269369#gistcomment-6269369)?
const gfx::Size min_size = helper()->browser_view()->GetMinimumSize();ilsubyeegaWe should also verify that tabbed PWAs do not suffer from overlapping elements (new tab button, extension button, origin text) at the new minimum width, which was the issue in b/40708657.
Could we add a test case or verify manually that the layout handles very narrow widths gracefully without overlaps?
Dibyajyoti PalI think I don't have any working tabbed PWA from my side at this time(Linux wayland), so I might need to get direction to figure out.
Will you be able to test out tabbed mode with some a test site like https://googlechrome.github.io/samples/pwa-testing/flossy-neon-sugar/? You might need to enable the 3 flags from chrome://flags for verifying tabbed mode:
- chrome://flags/#enable-desktop-pwas-tab-strip
- chrome://flags/#enable-desktop-pwas-tab-strip-settings
- chrome://flags/#enable-desktop-pwas-tab-strip-customizations
Either way, the UX requirement for anti-spoofing still remains.
[Heres the images](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269365#gistcomment-6269365). Looks like it's only height issue to ignore(I need to think other stuffs not to break).
Related details
- Daniel Murphy
- Dibyajyoti Pal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 2 comments![]( "Open in Gerrit")
Daniel Murphy@dmu...@chromium.org interested in what you think here.
Based on my understanding of the intent provided on [Github](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76), I don't think this is the correct behavior. The minimum width of the PWA window takes into account various security specific behavior of PWA windows, like enough width to show the origin text and animate it. From the intent, it seems like the window is too short for the origin to show up, so these changes are not correct.
At the very least, if you want the PWA windows to be shorter and not tied to the browser window's size, we should have enough space to show the origin text, the app's title and other icons on the toolbar to prevent users from being spoofed.
https://g-issues.chromium.org/issues/40614379#comment30 also says that the window size of PWAs were brought down to 150px, can you verify if that helps your use-case?
I could be wrong here though, in which case, happy to take back my -1, but this seems incorrect.
ilsubyeegaI agree - we need to make sure this doesn't cause issues with things like the origin text clipping. I think we scroll it - but yes. If there is a test we can have here around making sure that minimum content width includes also the toolbar stuff, then great. but I agree - there is a reason we did this.
Dibyajyoti Palwe should have enough space to show the origin text, the app's title and other icons on the toolbar to prevent users from being spoofed.
Would showing only icons, dropdown menu, window controls(- ㅁ X) work? Showing origin text(titles?) and app title would not able to work as it contains window controls by default(or maybe we can hide that and only show when hovered, but this should be addressed with separate issue)
https://g-issues.chromium.org/issues/40614379#comment30 also says that the window size of PWAs were brought down to 150px, can you verify if that helps your use-case?
This is re-landing of that idea but it contained multiple issues, find-in-page do not work, buttons overlapped etc. At that time it was extremely narrower(86px) than this patch does.
[image of chromium-145](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269339#gistcomment-6269339)
ilsubyeegaWould showing only icons, dropdown menu, window controls(- ㅁ X) work? Showing origin text(titles?) and app title would not able to work as it contains window controls by default(or maybe we can hide that and only show when hovered, but this should be addressed with separate issue)
Sadly no. The app title and the origin text HAS to show up on the PWA window afaik, because those are necessary anti-spoofing measures for the user.
I have question the sample image of the tabbed PWA demo does not enforce any title, URLs. Was it only used for testing purpose? May I get rechecked for this purpose? This patch directly challenges to anti-spoofing policy.
Or we can have workaround for retriving title and urls via hover to non-tabbed PWA like tabbed PWA current do. [image](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269370#gistcomment-6269370)
Hmmm, I do see that for tabbed PWAs, the title doesn't show up, but the origin text on the top right hand corner does animate for me. You can enable the flags mentioned, and relaunch the app post installation for the origin text animation to trigger on the top right hand corner. Just tested on Chrome Canary (v152.0.7964.0).
The window currently can be minimized to the exact width that is enough for the origin text animation to show, but I believe this CL is lowering that further, which isn't correct.
Or we can have workaround for retriving title and urls via hover to non-tabbed PWA like tabbed PWA current do. image
That is a workaround for sure (through hovering), but we cannot have the origin text be not showing up on the PWA window on launch, or be hidden during launch because the window is smaller. The best path forward is if you can come up with a solution that has enough space for the origin text to show up, and I think the latest code on Chromium already does this.
const gfx::Size min_size = helper()->browser_view()->GetMinimumSize();ilsubyeegaWe should also verify that tabbed PWAs do not suffer from overlapping elements (new tab button, extension button, origin text) at the new minimum width, which was the issue in b/40708657.
Could we add a test case or verify manually that the layout handles very narrow widths gracefully without overlaps?
Dibyajyoti PalI think I don't have any working tabbed PWA from my side at this time(Linux wayland), so I might need to get direction to figure out.
ilsubyeegaWill you be able to test out tabbed mode with some a test site like https://googlechrome.github.io/samples/pwa-testing/flossy-neon-sugar/? You might need to enable the 3 flags from chrome://flags for verifying tabbed mode:
- chrome://flags/#enable-desktop-pwas-tab-strip
- chrome://flags/#enable-desktop-pwas-tab-strip-settings
- chrome://flags/#enable-desktop-pwas-tab-strip-customizations
Either way, the UX requirement for anti-spoofing still remains.
[Heres the images](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269365#gistcomment-6269365). Looks like it's only height issue to ignore(I need to think other stuffs not to break).
The extra bit of UX on the tabbed windows right below the tab at the top of the window seems weird. Was it always there (even pre this CL)?
Related details
- Daniel Murphy
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ilsubyeega (Gerrit)
ilsubyeega added 2 comments![]( "Open in Gerrit")
Yeah, I can see the animations. While I'm looking for search for decent patching(some UX to fix & be enforced), How about make this as "removing minimum content width" experimental feature flag with disabled-default/opt-in? Looks like its still early to land this.
const gfx::Size min_size = helper()->browser_view()->GetMinimumSize();ilsubyeegaWe should also verify that tabbed PWAs do not suffer from overlapping elements (new tab button, extension button, origin text) at the new minimum width, which was the issue in b/40708657.
Could we add a test case or verify manually that the layout handles very narrow widths gracefully without overlaps?
Dibyajyoti PalI think I don't have any working tabbed PWA from my side at this time(Linux wayland), so I might need to get direction to figure out.
ilsubyeegaWill you be able to test out tabbed mode with some a test site like https://googlechrome.github.io/samples/pwa-testing/flossy-neon-sugar/? You might need to enable the 3 flags from chrome://flags for verifying tabbed mode:
- chrome://flags/#enable-desktop-pwas-tab-strip
- chrome://flags/#enable-desktop-pwas-tab-strip-settings
- chrome://flags/#enable-desktop-pwas-tab-strip-customizations
Either way, the UX requirement for anti-spoofing still remains.
Dibyajyoti Pal[Heres the images](https://gist.github.com/ilsubyeega/f53028e663bcd651b87ac9b709c38d76?permalink_comment_id=6269365#gistcomment-6269365). Looks like it's only height issue to ignore(I need to think other stuffs not to break).
The extra bit of UX on the tabbed windows right below the tab at the top of the window seems weird. Was it always there (even pre this CL)?
I cannot set say sure but it was done with official Google Chrome canary so(pre-CL), it was there when I researched for this review.
Related details
- Daniel Murphy
- Dibyajyoti Pal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
I'm going to refer the decision to @dmu...@chromium.org here, but I stand by the fact that it is not a good idea to make the current UX changes be available, even as an experimental feature behind a flag, unless it has gone through thorough UX and security review, primarily from our end that this change could result in potentially removing the app title and the origin text animation (the full url, without being occluded) from the app's header. Sorry about that!
Related details
- Daniel Murphy
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy added 1 comment![]( "Open in Gerrit")
Since we have no intention of ever shipping removing minimum width due to the security and trust issues (as well as UI breakage), we really don't want the maintenance cost of maintaining features like that. So I don't support adding a feature flag for this, no.
Related details
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ilsubyeega (Gerrit)
ilsubyeega added 1 comment![]( "Open in Gerrit")
I would like to remind that this minimum width behavior can be mitigated through javascript(`window.open`, without patching chromium, just official chrome binary), so still the issue exists. I simply do not want to go through that process.
Separately, there are some third-party products utilizing PWAs, and I would appreciate it if you could re-check the benefits of lower these restrictions. Otherwise, if there are any relevant forums or channels to advocate for this feature, please let me know.
Since it's my first CL, Really had great experiences. Thanks!
Related details
- Daniel Murphy
- Dibyajyoti Pal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
Thank you for your contributions here, and the thorough manual testing! You can file a new feature request for your ask using crbug.com/new, and use `UI > Browser > WebAppInstalls` as the component, so that it is routed to the PWA team. Feel free to reference this CL or comments in this CL in there as suggestions if you want. Just a heads up though, it seems like there is a workaround here (which is using `window.open()` which works for your use-case it seems), so it's likely that there might not be much work done here, unless there is a clear solution to the security issues that the feature request brings up.
If you're interested in continuing to contribute to Chromium, please let us know as well!
Related details
- Daniel Murphy
- ilsubyeega
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ilsubyeega (Gerrit)
ilsubyeega abandoned this change.
ilsubyeega abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |