Coder Social home page Coder Social logo

behaviors's Introduction

behaviors's People

Contributors

broccolinisoup avatar camertron avatar colebemis avatar dependabot[bot] avatar dgreif avatar enixcoda avatar francinelucca avatar github-actions[bot] avatar jbrown1618 avatar jellobagel avatar jonrohan avatar joshblack avatar keithamus avatar lesliecdubs avatar lukasoppermann avatar mperrotti avatar owenniblock avatar pksjce avatar primer-css avatar siddharthkp avatar tylerjdev avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar

behaviors's Issues

Focus Zone sets tabIndex to 0 for unfocusable element when it updates

Describe the bug
If an element is given a tabIndex of -1 and filtered out of a Focus Zone with focusableElementFilter, it should not participate in the Focus Zone or be tabbable. This works as expected until some prop on the unfocusable element changes. When that happens, the Focus Zone will update the tabIndex of that element to be 0, making it tabbable which not desired. This seems like an edge case, but it is occurring in the ReposFileTree component, which contains expand/collapse buttons in each row that should not participate in the Focus Zone.

Note: As this is a public repo, please be careful not to include details or screenshots from unreleased GitHub products or features. In most cases, a good bug report should be able to describe the new component without including business logic or feature details, but if you must discuss context relating to an unreleased feature, please open an issue in the private Design Systems repo and link to it here.

To Reproduce
Here's a simple example:

export const DefaultButton = (args: ButtonProps) => {
  const [expanded, setExpanded] = useState(false)
  const {containerRef} = useFocusZone({
    focusableElementFilter: element => element.getAttribute('data-focusable') !== 'false'
  })
  return (
    <Box ref={containerRef as RefObject<HTMLDivElement>}>
      <Button {...args}>Default</Button>
      <Button {...args}>Default2</Button>
      <Button
        {...args}
        data-focusable="false"
        tabIndex={-1}
        aria-expanded={expanded}
        onClick={() => {
          setExpanded(!expanded)
        }}
      >
        Default3
      </Button>
      <Button {...args}>Default4</Button>
    </Box>
  )
}
  1. Paste into the DefaultButton storyboook example in this repo
  2. Inspect the Default3 Button and notice it has a tabIndex of -1
  3. Click on the Default3 Button and notice its tabIndex changes to 0

Expected behavior
Elements that are being filtered out of the focus zone with focusableElementFilter should not have their tabIndex updated by it.

Desktop (please complete the following information):

  • OS: MacOS
  • Browser chrome
  • Version 103.0.5060.134 (Official Build) (x86_64)

Additional context
Add any other context about the problem here.

Focus zone calls error in vitest + jsdom

related - primer/react#2454

Description

This issue may better exist on primer/behaviors (or even upstreamed to jsdom if we can repro simply enough)

Initially reported in slack by @itsbagpack
https://github.slack.com/archives/C01L618AEP9/p1666186967969609

👋:skin-tone-2: hi friends, i’m running into some testing issues around using the Autocomplete component and i was wondering what we were missing
for context, here’s the PR we’re working with: https://github.com/github/delorean/pull/127

TypeError: Failed to execute 'addEventListener' on 'EventTarget': parameter 3 dictionary has member 'signal' that is not of type 'AbortSignal'.
❯ exports.convert node_modules/jsdom/lib/jsdom/living/generated/AddEventListenerOptions.js:53:11
❯ HTMLDivElement.addEventListener node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:139:46
❯ Module.focusZone node_modules/@primer/behaviors/dist/esm/focus-zone.js:248:15
   246|     });
   247|     let elementIndexFocusedByClick = undefined;
   248|     container.addEventListener('mousedown', event => {
      |               ^
   249|         if (event.target instanceof HTMLElement && event.target !== document.activeElement) {
   250|             elementIndexFocusedByClick = focusableElements.indexOf(event.target);
❯ node_modules/@primer/react/lib-esm/hooks/useFocusZone.js:21:34

It appears that jsdom is throwing when trying to add an event listener, and appears that's due to the primer/behaviors focusZone attempting to pass a signal, which appears to conflict with the accepted signal jsdom expects.

Primer/react uses jest (with jsdom) to test this component, which is confusing to me, given there's no indication that it currently has issues here

This leads me to think either
a version of JSDOM begins breaking
vite and jest somehow differ in the globals they send to jsdom in this scenario

I suggested a workaround, temporarily, mocking focusZone entirely -

vi.mock("@primer/behaviors", async () => {
 const behaviors = await vi.importActual<typeof import("@primer/behaviors")>(
   "@primer/behaviors",
 )

 return {
   ...behaviors,
   focusZone: vi.fn(),
 }
})

but this changes behavior of the autocomplete, and is not ideal

Steps to reproduce

start a project with vite/vitest
render an AutoComplete in a test using the jsdom environment

see jsdom throw

Version

All related versions of multiple packages

It's unclear to me whether this problem already exists and is becoming clear due to versioning of react and testing-libary, is due to something with how vite interops globals compared to jest, or if this is jest/jsdom.

"dependencies": {
    "@primer/octicons-react": "^17.7.0",
    "@primer/react": "^35.11.0-rc.65a8bb8a",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "styled-components": "^5.3.5"
  },
  "devDependencies": {
    "@testing-library/jest-dom": "^5.16.5",
    "@testing-library/react": "^13.4.0",
    "@testing-library/user-event": "^14.4.3",
    "@types/react": "^18.0.17",
    "@types/react-dom": "^18.0.6",
    "@vitejs/plugin-react": "^2.0.1",
    "typescript": "^4.6.4",
    "vite": "^3.0.7",
    "vitest": "^0.23.4"
  }

Browser

No response

`getAnchoredPosition` returns position without perfectly aligned edges

Context

We use getAnchoredPosition for primer-tooltip positioning (which is soon to be upstreamed to PVC).

In some scenarios, we observed the tooltip is not perfectly aligned at edge, particularly when there's no space for perfect edge alignment. Here is an example where align is end.

Screenshot of tooltip edge not perfectly aligned at edge of anchor element

I was also able to recreate this issue in storybook:

Screenshot of storybook example where floating element edge is not perfectly aligned at edge of anchor element

Expected behavior

Since there’s no space to perfectly align end, I would have expected anchorAlignment to be adjusted to start instead. Is that not expected behavior?

For now, we're thinking of housing additional positioning logic in tooltip to address immediate issues, but this should probably be responsibility of getAnchoredPosition.

Focusing into a focus zone from detached focus does not use provided `focusInStrategy` function

NOTE: For lack of a better term here, when I refer to detached focus I mean when no specific element is focused but the browsers knows where in the focus flow the user should be. For example, this can happen when the currently focused element is unmounted or disabled, or if the element is the first focuseable item on the page.

When tabbing from a detached focus state to a focus zone that has a focusInStrategy function set, the function is not called and thus the first focuseable element in the zone is always focused. This appears to be because the function is only called if event.relatedTarget instanceof HTMLElement but there is no relatedTarget in this case (relatedTarget is null). It looks like the check may just be here to satisfy TypeScript:

if (event.relatedTarget instanceof Element && !container.contains(event.relatedTarget)) {

Here is a minimal demo of an instance where relatedTarget can be null: https://jsfiddle.net/9cn7zxaq/

Here is an example of an instance where this causes a problem. I am building a datepicker and when I press the "Today" button, it is disabled because the picker moves to today's date. Focus is lost, then if the user Shift+Tabs to the days grid, I have a focusInStrategy that will set the focus to the correct day. However, my strategy is not called and the focus is set to the first day of the month instead.

Screen.Recording.2022-02-28.at.2.51.34.PM.mov

Focus Zone, moving focus stops working upwards if hidden attribute is set to true of an element in the focus flow

Moving focus stops working upwards if hidden attribute is set to true of an element in the focus flow

To reproduce:

I have button that I hide when it is clicked

Start moving focus downwards till button is reached, click that button, click handler will execute button.hidden = true;
moving focus downwards if further elements are present below the now hidden button works
moving focus upwards from where the button was previously displayed no longer works

So setting an element to hidden in the chain, disrupts the focus flow upwards starting from that element

Replace behaviors in PRC

Once the behaviors are extracted, we should remove the duplicate code from primer/react and import them from here

Add support for contenteditable=true in iterateFocusableElements

Elements with contenteditable=true are focusable natively in the browser. We would like for iterateFocusableElements method to also account for contenteditable when checking for focusable elements

MDN: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contenteditable

Snippet in iterate-focusable-elements.ts where focus is checked: https://github.com/primer/behaviors/blob/main/src/utils/iterate-focusable-elements.ts#L86

`getAnchoredPosition` does not respect `margin` on document body

Hi, I noticed that the tooltips in PR pages are not aligned properly when body has margin set.

Check out the comparison below, you can see the tooltip shifts 200px relative to body between the 2 cases. And 200px is exactly the margin-left set to body.

no margin on body Manually add 200px margin on body
image image

Why would body has margin?

I noticed this issue because I use a GitHub browser extension that adds a sidebar to the page and set margin to body so that the sidebar would not overlap page content. I understand that primer is not responsible for handling of user's browser extension use case. But perhaps my use case has revealed a tiny defect of primer/behaviors and would help improve it. :)

image

Some insights I have noticed

Tooltip's position was set by

https://github.com/primer/view_components/blob/81eea01d0034c55771ce3e6c2afc685d364afc6c/app/components/primer/alpha/tool_tip.ts#L416-L417

And the position was calculated from getAnchoredPosition

export function getAnchoredPosition(

In getAnchoredPosition, the relativeRect has the correct left value for tooltip, if it was set to tooltip's left style, it would align properly.

const relativeRect = {
top: parentElementRect.top + borderTop,
left: parentElementRect.left + borderLeft,
}

However, the relativeRect was then passed to pureCalculateAnchoredPosition and it returned a different left.

function pureCalculateAnchoredPosition(
viewportRect: BoxPosition,
relativePosition: Position,
floatingRect: Size,
anchorRect: BoxPosition,
{side, align, allowOutOfBounds, anchorOffset, alignmentOffset}: PositionSettings,
): AnchorPosition {
// Compute the relative viewport rect, to bring it into the same coordinate space as `pos`
const relativeViewportRect: BoxPosition = {
top: viewportRect.top - relativePosition.top,
left: viewportRect.left - relativePosition.left,
width: viewportRect.width,
height: viewportRect.height,
}
let pos = calculatePosition(floatingRect, anchorRect, side, align, anchorOffset, alignmentOffset)
let anchorSide = side
let anchorAlign = align
pos.top -= relativePosition.top
pos.left -= relativePosition.left
// Handle screen overflow
if (!allowOutOfBounds) {
const alternateOrder = alternateOrders[side]
let positionAttempt = 0
if (alternateOrder) {
let prevSide = side
// Try all the alternate sides until one does not overflow
while (
positionAttempt < alternateOrder.length &&
shouldRecalculatePosition(prevSide, pos, relativeViewportRect, floatingRect)
) {
const nextSide = alternateOrder[positionAttempt++]
prevSide = nextSide
// If we have cut off in the same dimension as the "side" option, try flipping to the opposite side.
pos = calculatePosition(floatingRect, anchorRect, nextSide, align, anchorOffset, alignmentOffset)
pos.top -= relativePosition.top
pos.left -= relativePosition.left
anchorSide = nextSide
}
}
// If using alternate anchor side does not stop overflow,
// try using an alternate alignment
const alternateAlignment = alternateAlignments[align]
let alignmentAttempt = 0
if (alternateAlignment) {
let prevAlign = align
// Try all the alternate alignments until one does not overflow
while (
alignmentAttempt < alternateAlignment.length &&
shouldRecalculateAlignment(prevAlign, pos, relativeViewportRect, floatingRect)
) {
const nextAlign = alternateAlignment[alignmentAttempt++]
prevAlign = nextAlign
pos = calculatePosition(floatingRect, anchorRect, anchorSide, nextAlign, anchorOffset, alignmentOffset)
pos.top -= relativePosition.top
pos.left -= relativePosition.left
anchorAlign = nextAlign
}
}
// At this point we've flipped the position if applicable. Now just nudge until it's on-screen.
if (pos.top < relativeViewportRect.top) {
pos.top = relativeViewportRect.top
}
if (pos.left < relativeViewportRect.left) {
pos.left = relativeViewportRect.left
}
if (pos.left + floatingRect.width > viewportRect.width + relativeViewportRect.left) {
pos.left = viewportRect.width + relativeViewportRect.left - floatingRect.width
}
// If we have exhausted all possible positions and none of them worked, we
// say that overflowing the bottom of the screen is acceptable since it is
// likely to be able to scroll.
if (alternateOrder && positionAttempt < alternateOrder.length) {
if (pos.top + floatingRect.height > viewportRect.height + relativeViewportRect.top) {
pos.top = viewportRect.height + relativeViewportRect.top - floatingRect.height
}
}
}
return {...pos, anchorSide, anchorAlign}
}


Thank you for this awesome project!

Set up Doctocat

We need to set up real docs for this project. We can pull the existing docs out of PRC and add a new "Behaviors" section to primer.styles

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.