Improve narrow width iframes #273

GioSensation · 2023-03-15T08:36:18Z

Reviewer: @alistairjcbrown
Asana: https://app.asana.com/0/0/1204100616987020/f

Description

On extensions, our tooltip could be cut off in iframes. This attempts to mitigate the issue by centering the tooltip when the left border is out of the visible frame and when it's overflowing from the right side it just moves it slightly inwards.

Steps to test

Automated tests added. To see it in action you can check the iframe signup form at reddit.com and https://gravida.pro/nytimes/New%20York%20Times%20Checkout (note that I uploaded the form to one of my sites because nytimes is pretty aggressive with bot blocking and I wasn't able to test after just a couple of refreshes).

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

…b.com:duckduckgo/duckduckgo-autofill into ema/improve-narrow-width-iframes Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> # Conflicts: # dist/autofill-debug.js # dist/autofill.css # dist/autofill.js # swift-package/Resources/assets/autofill-debug.js # swift-package/Resources/assets/autofill.css # swift-package/Resources/assets/autofill.js

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation · 2023-03-15T12:29:57Z

integration-test/pages/email-autofill.html

@@ -14,7 +14,7 @@
 <p id="demo"></p>

 <div>
-  Small inputs should be ignored: <input id="email" type="email" style="width: 30px;">
+  Small inputs should be ignored: <input id="emailSmall" type="email" style="width: 30px;">


Nothing to see. Just two elements with the same id.

GioSensation · 2023-03-15T12:31:30Z

package.json

@@ -35,7 +35,7 @@
    "@babel/preset-env": "^7.16.11",
    "@duckduckgo/content-scope-scripts": "github:duckduckgo/content-scope-scripts#1.3.0",
    "@duckduckgo/content-scope-utils": "github:duckduckgo/content-scope-utils#1.0.1",
-    "@playwright/test": "^1.24.1",
+    "@playwright/test": "^1.31.2",


Had to bump playwright to use expect.toBeInViewport().

GioSensation · 2023-03-15T12:32:06Z

src/Form/selectors-css.js

@@ -22,6 +22,8 @@ input[type=email],
 input[type=text][aria-label*=email i]:not([aria-label*=search i]),
 input:not([type])[aria-label*=email i]:not([aria-label*=search i]),
 input[name=username][type=email],
+input[autocomplete=username][type=email],
+input[autocomplete=username][placeholder*=email i],


Unrelated, but while testing I noticed a regression on tripadvisor (form in iframe) and fixed it.

GioSensation · 2023-03-15T12:32:50Z

src/UI/EmailHTMLTooltip.js

@@ -24,6 +24,7 @@ ${this.options.css}
            <span class="tooltip__button--email__secondary-text">Blocks email trackers and hides your address</span>
        </button>
    </div>
+    <div class="tooltip--email__caret"></div>


The caret used to be a pseudo element, but now that we're positioning them independently, I created a separate element.

GioSensation · 2023-03-15T12:33:42Z

src/UI/HTMLTooltip.js

+            },
+            tooltip: {
+                getRuleString: this.options.tooltipPositionClass,
+                index: null


This allows us to keep two separate indexes for the rules we add to the stylesheet, so we can delete them appropriately.

GioSensation · 2023-03-15T12:35:04Z

src/UI/HTMLTooltip.js

+            shadow.styleSheets[0].insertRule(cssRule, ruleObj.index)
+        }
+
+        if (this.options.hasCaret) {


We only check overflowing if the tooltip has the caret, because on desktop apps the tooltip sits on top of the page so is not influenced by iframes and such.

GioSensation · 2023-03-15T12:38:44Z

src/UI/HTMLTooltip.js

+                const overriddenLeftPosition = left - Math.abs(rightOverflow) - 5
+
+                this.updatePosition(element, {left: overriddenLeftPosition, top})
+            }


I tried several combinations of these and they work pretty decently. It's certainly an improvement over the previous state. It never gets into an infinite loop because we constrain the width to the viewport so it can never overflow both ends. I tried removing the rule and the test fails so we're covered.

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation · 2023-03-15T12:41:34Z

src/UI/styles/autofill-tooltip-styles.css

@@ -14,8 +14,6 @@
    BlinkMacSystemFont, 'Segoe UI', 'Roboto', 'Oxygen', 'Ubuntu',
    'Cantarell', 'Fira Sans', 'Droid Sans', 'Helvetica Neue', sans-serif;
    -webkit-font-smoothing: antialiased;
-    /* move it offscreen to avoid flashing */
-    transform: translate(-1000px);


I don't think this was needed any longer because now the tooltip is hidden before it's positioned so we should be good. I tried testing slowing down the CPU as well, so I think we're fine, but let me know if you notice anything weird.

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

alistairjcbrown

A couple of comments, but nothing blocking. LGTM

alistairjcbrown · 2023-03-15T14:10:58Z

integration-test/helpers/pages.js

+            const input = await page.frameLocator('iframe').locator('input#email')
+            const box = await input.boundingBox()
+            if (!box) throw new Error('unreachable')
+            await input.click({position: {x: box.width - (box.height / 2), y: box.height / 2}})


NAB: Worth updating the existing clickDirectlyOnDax helper to take an optional input parameter to keep the position logic in one place?

alistairjcbrown · 2023-03-15T14:11:55Z

integration-test/helpers/pages.js

+        async assertTooltipWithinFrame () {
+            const tooltip = await page.frameLocator('iframe').locator('.tooltip--email')
+            await expect(tooltip).toBeVisible()
+            await expect(tooltip).toBeInViewport({ ratio: 1 })


Nice assertion!

alistairjcbrown · 2023-03-15T21:21:34Z

src/UI/HTMLTooltip.js

@@ -93,13 +119,22 @@ export class HTMLTooltip {
            const {left, bottom} = this.getPosition()

            if (left !== this.left || bottom !== this.top) {
-                this.updatePosition({left, top: bottom})
+                this.updatePositions({left, top: bottom})


NAB: may be worth just inlining updatePositions here for readability, and avoid having two very similarly named functions

alistairjcbrown · 2023-03-16T10:56:22Z

src/UI/HTMLTooltip.js

+                const rightOverflow = window.innerWidth - tooltipBoundingBox.right
+                const overriddenLeftPosition = left - Math.abs(rightOverflow) - 5


NAB: We can remove the Math.abs call by flipping the subtraction

Suggested change

const rightOverflow = window.innerWidth - tooltipBoundingBox.right

const overriddenLeftPosition = left - Math.abs(rightOverflow) - 5

const rightOverflow = tooltipBoundingBox.right - window.innerWidth

const paddingSpace = 5

const overriddenLeftPosition = left - rightOverflow - paddingSpace

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

* Prevent in-context signup showing on local or invalid domain pages (#263) * Don't open in-context signup on input click (#264) * Show grayscale Dax icon for in-context signup (#265) * Remove initial dismissal in favor of single permanent dismissal (#266) * Only show in-context signup if recently installed (#267) * Adds a close X button to the top right of the tooltip (#268) * Fix grayscale dax logo (#271) * Ema/prevent focus extensions (#270) * Update icon asset to fix grayscale version (#276) * Improve narrow width iframes (#273) * Add Email Protection tooltip pixel (#277) * Fade Dax icon in on hover for in-context signup (#278) * Fade Dax icon in on hover for in-context signup * Keep Dax icon with active styles when tooltip open * Prevent icon flicker when clicking Dax icon for open tooltip * Move style management into form * Position tooltip above input when no space below (#282) * Position tooltip above input when no space below * Preload webfonts before showing tooltip This prevents the fonts loading in once the tooltip is shown and causing a layout shift. This is more important when the tooltip is showing above the input, as it can cause the tooltip to jump up. * Check tooltip position after revealed and bail if called before When running on slower devices, the tooltip position may be checked before the tooltip is revealed. When this happens, we now bail early without updating the position. Check position is now called after styles have loaded, when we know the tooltip is visible. * Hide caret initially and reduce position calculation calls * Read hidden attribute from DOM Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> * Fix linting issue with playwright report Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> * Rename typoed file Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> --------- Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> Co-authored-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> --------- Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> Co-authored-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation added 9 commits March 14, 2023 10:40

Decouple caret position from tooltip container

09e17e3

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Commit progress before changing everything

36f0445

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Minor selector improvements

668fccb

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Use option hasCaret

83abb8b

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Compile code after pull

333352d

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Tweak caret positioning

bfed2c7

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Add tests

2974934

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Handle right-side overflows graciously

98a3374

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation commented Mar 15, 2023

View reviewed changes

Remove obsolete comment

97ad460

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation commented Mar 15, 2023

View reviewed changes

GioSensation self-assigned this Mar 15, 2023

GioSensation requested a review from alistairjcbrown March 15, 2023 12:42

GioSensation marked this pull request as ready for review March 15, 2023 12:42

Commit compiled code

edb40c3

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

alistairjcbrown approved these changes Mar 16, 2023

View reviewed changes

Address PR feedback

06ddd7f

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation merged commit fa21349 into abrown/changes-to-in-context-signup-treatment Mar 17, 2023

GioSensation deleted the ema/improve-narrow-width-iframes branch March 17, 2023 08:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve narrow width iframes #273

Improve narrow width iframes #273

GioSensation commented Mar 15, 2023 •

edited

Loading

GioSensation Mar 15, 2023

GioSensation Mar 15, 2023

GioSensation Mar 15, 2023 •

edited

Loading

GioSensation Mar 15, 2023

GioSensation Mar 15, 2023

GioSensation Mar 15, 2023

GioSensation Mar 15, 2023

GioSensation Mar 15, 2023

alistairjcbrown left a comment

alistairjcbrown Mar 15, 2023 •

edited

Loading

alistairjcbrown Mar 15, 2023

alistairjcbrown Mar 15, 2023

alistairjcbrown Mar 16, 2023

		const rightOverflow = window.innerWidth - tooltipBoundingBox.right
		const overriddenLeftPosition = left - Math.abs(rightOverflow) - 5

-                const rightOverflow = window.innerWidth - tooltipBoundingBox.right
-                const overriddenLeftPosition = left - Math.abs(rightOverflow) - 5
+                const rightOverflow = tooltipBoundingBox.right - window.innerWidth
+                const paddingSpace = 5
+                const overriddenLeftPosition = left - rightOverflow - paddingSpace

Improve narrow width iframes #273

Improve narrow width iframes #273

Conversation

GioSensation commented Mar 15, 2023 • edited Loading

Description

Steps to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GioSensation Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alistairjcbrown left a comment

Choose a reason for hiding this comment

alistairjcbrown Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GioSensation commented Mar 15, 2023 •

edited

Loading

GioSensation Mar 15, 2023 •

edited

Loading

alistairjcbrown Mar 15, 2023 •

edited

Loading