Closed Bug 947073 Opened 6 years ago Closed 6 years ago

Phishing Warning hides "Why is this page blocked" under "ignore warning"

Categories

(Firefox for Android :: Theme and Visual Design, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed
fennec + ---

People

(Reporter: devd, Assigned: wesj)

References

Details

Attachments

(3 files, 1 obsolete file)

See screenshot attached.

If anything, we should make ignore hard to click over the other buttons like "why is this blocked". I don't know why ignore is drawing over other buttons.
Adding wesj who worked on it in Bug 808636.
See Also: → 808636
Summary: Phishing Warning hides "Why is this page blocked" over "ignore warning" → Phishing Warning hides "Why is this page blocked" under "ignore warning"
Ignore is fixed position bottom. It will always draw over the top of things if the device isn't tall enough to show everything. I added some padding to the bottom of the page to try and make sure you could scroll in these situations. Is it possible to scroll?
Can't scroll either. Weirdly enough, it all works great if I go to my phone's homescreen and open Firefox again: now, ignore is below the "why is this page blocked" (and invisible under the page fold). I an scroll down and click on it.

I am not sure why ignore is fixed position bottom to draw over top of other things? Presumably, we don't want people to go to phishing sites? Note how small and in the corner the "ignore" is for Desktop Firefox.
I didn't want to push it to a potentially off screen location. We can see what UX thinks, but we should at least fix the scrolling behavior.
Flags: needinfo?(ibarlow)
Hm, I always envisioned this page to scroll, and not to have "ignore" fixed to the bottom like this, but rather it would be under the buttons which means a user would probably have to scroll to get to it.
Flags: needinfo?(ibarlow)
tracking-fennec: --- → ?
Assignee: nobody → wjohnston
tracking-fennec: ? → +
Attached patch PatchSplinter Review
This uses flex-box to give us a sticky bottom link (position: sticky always stays on screen, so it doesn't give you what you want here).
Attachment #8359460 - Flags: review?(margaret.leibovic)
Comment on attachment 8359460 [details] [diff] [review]
Patch

Review of attachment 8359460 [details] [diff] [review]:
-----------------------------------------------------------------

I don't want to be too picky about this CSS, but I do have some questions...

::: mobile/android/themes/core/netError.css
@@ +150,5 @@
>     bottom. This is preferable to just using a text link
>     since there is already a mechanism in browser.js for trapping
>     oncommand events from unprivileged chrome pages (ErrorPageEventHandler).*/
>  #ignoreWarningButton {
> +  width: calc(100% + 40px);

Why the + 40px here?

@@ +155,5 @@
> +  -moz-appearance: none;
> +  background: #b14646;
> +  border: none;
> +  text-decoration: underline;
> +  margin: 0 0 0 -20px;

Why just a negative margin on the left?

@@ +157,5 @@
> +  border: none;
> +  text-decoration: underline;
> +  margin: 0 0 0 -20px;
> +  font-size: smaller;
> +  border-bottom: none;

You shouldn't need this, since you say border: none; up above (I realize the code was already like this, but we might as well fix it now).

@@ +158,5 @@
> +  text-decoration: underline;
> +  margin: 0 0 0 -20px;
> +  font-size: smaller;
> +  border-bottom: none;
> +  border-radius: 0;

Why do you need a border-radius here?
(In reply to :Margaret Leibovic from comment #7)
> >  #ignoreWarningButton {
> > +  width: calc(100% + 40px);
> Why the + 40px here?

This is inside a container that has a 20px padding around it. This button has to extend edge to edge on the screen though, so I added 20px*2 to its width. We could use CSS variables to make this a little prettier/easier to read.
 
> > +  margin: 0 0 0 -20px;

> Why just a negative margin on the left?

And this offsets it so that it sits edge to edge (20px padding - 20px margin). I want to avoid changing the markup here, because I'd like to kill our special files and share with desktop (where the button sits inside the box). So this is a bit of a hacky way to make it right.

> You shouldn't need this, since you say border: none; up above (I realize the
> code was already like this, but we might as well fix it now).

Good catch. Bad wes.

> Why do you need a border-radius here?
This is a button, and we style other buttons on this page with a 2px border radius. This removes it so that you don't see a tiny bit of color at the bottom edges of this.
(In reply to Wesley Johnston (:wesj) from comment #8)
> (In reply to :Margaret Leibovic from comment #7)
> > >  #ignoreWarningButton {
> > > +  width: calc(100% + 40px);
> > Why the + 40px here?
> 
> This is inside a container that has a 20px padding around it. This button
> has to extend edge to edge on the screen though, so I added 20px*2 to its
> width. We could use CSS variables to make this a little prettier/easier to
> read.

Cool, I didn't know about the padding, that explains things.

> > > +  margin: 0 0 0 -20px;
> 
> > Why just a negative margin on the left?
> 
> And this offsets it so that it sits edge to edge (20px padding - 20px
> margin). I want to avoid changing the markup here, because I'd like to kill
> our special files and share with desktop (where the button sits inside the
> box). So this is a bit of a hacky way to make it right.

Are we worried about RTL support? Perhaps a better way to do this would be margin: 0; -moz-margin-start: -20px; (or maybe you don't even need that margin: 0; if there's no margin to begin with).
Attachment #8359460 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8359460 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 808636
User impact if declined: This button can obscure other buttons on the phishing pages, making them difficult (impossible?) to click
Testing completed (on m-c, etc.): Landed on mc now.
Risk to taking this patch (and alternatives if risky): Pretty low risk CSS change. We're already shipping the bug. We could let his ride the trains, or just remove position:absolute here to style this differently.
String or IDL/UUID changes made by this patch: none.
Attachment #8359460 - Flags: approval-mozilla-beta?
Attachment #8359460 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/74335bb5706a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Comment on attachment 8359460 [details] [diff] [review]
Patch

low risk and easily verifiable so let's take it
Attachment #8359460 - Flags: approval-mozilla-beta?
Attachment #8359460 - Flags: approval-mozilla-beta+
Attachment #8359460 - Flags: approval-mozilla-aurora?
Attachment #8359460 - Flags: approval-mozilla-aurora+
I did this wrong and now the try again button on normal error pages is pushed to the bottom. Fixing...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Follow up (obsolete) — Splinter Review
This is just more specific about when to flex and when not to. I'm not sure I love it, but I continue to not want to change the markup. You have any opinion margaret?
Attachment #8363306 - Flags: review?(margaret.leibovic)
Attached patch PatchSplinter Review
Non-empty patch
Attachment #8363306 - Attachment is obsolete: true
Attachment #8363306 - Flags: review?(margaret.leibovic)
Attachment #8363849 - Flags: review?(margaret.leibovic)
Comment on attachment 8363849 [details] [diff] [review]
Patch

Review of attachment 8363849 [details] [diff] [review]:
-----------------------------------------------------------------

Disclaimer: I didn't test this. But this seems like a reasonable fix to me, I agree that it's good to avoid changing the markup so that we can share it.
Attachment #8363849 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/79b5171fbd02
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.