The accessibility bug below is about lost keyboard focus after using
keyboard to edit style rules. It reveals a flaw in the style editing
code: it is justifiable to call "toggle property and continue editing"
when the property is being typed. But when the command handler realizes
that we need to cancel this editing, the following problems occured.
1) The cancel editing code is called before the necessary information
for continuing editing is gathered. An exception can be thrown. The fix
orders them correctly.
2) The continue editing code does not set the focuses correctly. There
are three situations:
2.a) the canceled editing is in the middle of a list of properties;
focus/editing-starting position should be at the next property.
2.b) the canceled editing is at the end of a list of properties; it
should be reasonable to set focus/editing-starting position at the
last property.
2.c) the section has no css rules; it is reasonable to set focus on
the section itself.
Screen recording for 2.a and 2.c:
continue editing the next property:
https://imgur.com/VFJjhE2
no property in section, focus on section:
https://imgur.com/mlhgo5t
Change-Id: Ic8c41ef3acc4f55331356b651aabcf7dc90ca43e
Bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1081766
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2196823
Reviewed-by: Vidal Diazleal <vidorteg@microsoft.com>
Reviewed-by: Brian Cui <brcui@microsoft.com>
Commit-Queue: Songtao Xia <soxia@microsoft.com>
Issue:
- When deleting a StylePropertyTreeElement via an empty string, the updatedProperty variable (which is eventually set to this.property) is incorrectly set.
- this._style removes the deleted property on line 1145 at "let success = await this.property.setText(styleText, majorChange, overwriteProperty);"
- When fetching updatedProperty via this._style.propertyAt(this.property.index), this.property.index is not adjusting for the deleted property
- This results in setting updatedProperty to an incorrect property (usually the property after the deleted one) or null (if you are deleting the last property in the section).
Changes:
- Adding check if the property is a majorChange (committing changes) and if the styleText is empty (will delete property)
- In this case, it will keep the property as is to avoid setting it to null or another property.
Bug: 1077097
Change-Id: I76d13fc78ef02c2a85e3b9875ec7c763d313508f
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2175085
Reviewed-by: Tony Ross <tross@microsoft.com>
Reviewed-by: Vidal Diazleal <vidorteg@microsoft.com>
Commit-Queue: Michael Liao <michael.liao@microsoft.com>
This CL changes references to self.Bindings.cssWorkspaceBinding (the
globalinstance of Bindings.CSSWorkspaceBinding.CSSWorkspaceBinding) over
to Bindings.CSSWorkspaceBinding.CSSWorkspaceBinding.instance(). To keep
both TypeScript and Closure happy we must make a method on the
CSSWorkspaceBinding class itself, since it only allows private
constructors to be accessed by static methods on the class.
Bug: 1058320
Change-Id: Ic42d2b76e5bcec38029827dce14e3bf06ceaf89a
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2107520
Reviewed-by: Tim van der Lippe <tvanderlippe@chromium.org>
Commit-Queue: Paul Lewis <aerotwist@chromium.org>
This CL changes references to self.Common.settings (the global
instance of SDK.Common.Settings) over to
Common.Settings.Settings.instance(). To keep both TypeScript and
Closure happy we must make a method on the Settings class itself,
since it only allows private constructors to be accessed by static
methods on the class.
Bug: 1058320
Change-Id: I04afc8caf64acf29cdda13ef03ad05cfff4786a1
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2091450
Commit-Queue: Paul Lewis <aerotwist@chromium.org>
Reviewed-by: Tim van der Lippe <tvanderlippe@chromium.org>
It also makes sure that project references are working correctly between
`ts_library` definitions and adds a `global-defs.d.ts` file for platform
to define the prototype wrangling we are doing.
DISABLE_THIRD_PARTY_CHECK=Update config
Bug: 1011811
Change-Id: Ie3c63ea182803c815f0876ca147c9a8f6709d5e4
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2011080
Commit-Queue: Tim van der Lippe <tvanderlippe@chromium.org>
Reviewed-by: Mathias Bynens <mathias@chromium.org>
There are minor race conditions in which the StylePropertyTreeElement
is tracking its own CSSProperty and the property is deleted from the
CSSStyleDeclaration to which it belongs. The conditions are particularly
difficult to reproduce, and the user experience does not seem to raise
an issue, probably because changes to the underlying SDK model cause
immediate refreshes of the entire tree).
This change modifies the behavior to not cause an error in the event
that the race condition is encountered by treating edits as having the
entire string, so that user edits aren't dropped.
Change-Id: I2777196cb88fa2928f8a5471398b0a58c8c67853
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/1947523
Reviewed-by: Mike Jackson <mjackson@microsoft.com>
Commit-Queue: Robert Paveza <Rob.Paveza@microsoft.com>
By presenting an interstitial dialog (like Ctrl+P), keyboarding
users might get to a point where they can't put focus back into
the styles pane. This is because tabIndex is set to -1 for all
of the style blocks. Typically, losing focus causes the styles
list to reset the first style block to tabIndex=0, but only if
the style container doesn't already have focus. Because of an
event ordering issue, the styles still have focus before the
interstitials receive focus. This change allows the styles tree
elements to reset the styles when they lose focus.
Bug: 963183
Change-Id: I227c48fd88dbe46bbebc982b0c6e10232769d860
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1842521
Reviewed-by: Lorne Mitchell <lomitch@microsoft.com>
Commit-Queue: Robert Paveza <Rob.Paveza@microsoft.com>
Cr-Original-Commit-Position: refs/heads/master@{#704038}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 861c5ebd949d009975ded5e35ea5c4b710171d32
The Chromium/Google style guides does not enforce curly braces for
single-line if-statements, but does strongly recommend doing so. Adding
braces will improve code readability, by visually separating code
blocks. This will also prevent issues where accidental additions are
pushed to the "else"-clause instead of in the if-block.
This CL also updates the presubmit `eslint` to run the fix with the
correct configuration. It will now fix all issues it can fix.
Change-Id: I4b616f21a99393f168dec743c0bcbdc7f5db04a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1821526
Commit-Queue: Tim Van der Lippe <tvanderlippe@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Jeff Fisher <jeffish@microsoft.com>
Cr-Original-Commit-Position: refs/heads/master@{#701070}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 7e0bdbe2d7f9fc2386bfaefda3cc29c66ccc18f9
Previously, there was a property order mismatch between the DevTools
Console Preview and the expanded view. For example:
input> ({ c: 1, b: 1, a: 1 })
preview> {c: 1, b: 1, a: 1}
expanded> {a: 1, b: 1, c: 1, __proto__: Object}
This patch removes the confusing mismatch as follows:
input> ({ c: 1, b: 1, a: 1 })
preview> {c: 1, b: 1, a: 1}
expanded> {c: 1, b: 1, a: 1, __proto__: Object}
The core change happens in ObjectPropertiesSection.js. The test
expectations are updated accordingly (note that for each test, only
the order in which properties/values are printed changes). The patch
also includes a drive-by change across files unifying the way U+00A0
is escaped in string literals.
Screenshot: https://goo.gle/devtools-property-order-mismatch
BUG=989514
Change-Id: I102afcbd107100a52e0932401429a3fa8db99eaf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1806457
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Mathias Bynens <mathias@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#697200}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: dcc366c27c78ce40ca6f05ac8000528896b4549a
Accessibility testing revealed the following issues within the tool:
1. keyboard users cannot toggle the style properties
2. keyboard users cannot reveal in source for the style properties
This change adds:
add two items in context menu for edit mode so that keyboard user
can perform those two tasks described above
Bug: 963183
Workflow gif: https://imgur.com/a/to0eMPe
Change-Id: I33c66548e8bf2b89011a78ccb59ecd96a7ec60d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1670411
Commit-Queue: Junyi Xiao <juxiao@microsoft.com>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#691696}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: dab5ac2daa0c8c2002174d59eaa1f7c16551d948
The code we have to populate a tree uses await, except that in the recursive
case we don't await its Promise, instead treating it as a sync op. This resulted
in the bug that the child list is empty when checked. This patch updates that
code such that it now awaits the async populate process before checking for
child nodes to expand.
Bug: 961141
Change-Id: I35f3aacf3ef67d0de9a0c35a0c98ec500b08df58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1657907
Reviewed-by: Erik Luo <luoe@chromium.org>
Commit-Queue: Paul Lewis <aerotwist@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#672485}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 615fb1ff80242114d396b76db184953376b17fea
SSP used to use _hasBeenEditedIncrementally to know whether a
property's name or value was dirty. It would skip cleanup if it was
not dirty.
A recent change made _hasBeenEditedIncrementally() reset when moving
focus from name > value editor, which broke the cleanup phase.
This CL re-adds a dirty flag state.
Bug: 968792
Change-Id: I0b3b167d88ce2ee20e222e3e23d7accf179cebfb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1637608
Reviewed-by: Andrey Lushnikov <lushnikov@chromium.org>
Commit-Queue: Erik Luo <luoe@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#664974}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 1597dcc9dc35ab6cfe0944480fad435b3ddb3c05
This CL does not change behavior, but introduces a contextual
'_applyOriginalStyle()' to prepare for free-flow name editing:
- Used when selecting a non-applyable name in SuggestBox
- Reverting to the original style now restores the
'originalProperty'.
- If a user free-flows, then restores the original
(only possible when free-flowing names), we still should
'updateTitle()'. In this CL, we do it explicitly in
editCancelled.
Bug: 931145
Change-Id: I88a2a6feac688c09ed232b8ea2aa4b94d898590d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1548584
Reviewed-by: Joel Einbinder <einbinder@chromium.org>
Commit-Queue: Erik Luo <luoe@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#661637}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: c535e51520fd6c41c84f0da9485bda4899d90c59
A) SSP used to manually set height, 'contain: strict' during edits
for performance. This CL removes them, since there is no
noticeable gain. See crbug.com/684341
B) Removes unnecessary 'editing state tracking', once used to
manually focus sections before ShadowDOM-delegatesFocus.
See also crbug.com/791494
Pre-keyboard navigation) SSP has tabIndex 0. On editingCommitted,
we ensure focus goes back to the pane.
After-keynav) Individual sections have tabIndex -1. On stopEditing,
parent pane has focus, but we manually move focus to the section.
After ShadowDOM V1) parent pane no longer has tabIndex, but uses
'delegatesFocus'. This makes moving focus around unnecessary.
However, it also introduces 'blue flickering' when users 'Tab'
navigate through properties, since focus is first delegated to
the section. This bug is not addressed in this CL.
Bug: 931145
Change-Id: If24177344ec975478fd91937daea58630b396069
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1574366
Commit-Queue: Erik Luo <luoe@chromium.org>
Reviewed-by: Joel Einbinder <einbinder@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#652667}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 7a281343ffbccad662f6d3dce487f40bc926de1a
This patch starts displaying color swatches in suggest box
next to css variables that compute to color value.
For this to be possible, the ColorSwatch and BezierSwatch were
moved under the ui/ module. The CSSShadowSwatch stayed in the
inline_editor/ for now because it has a dependency on
ShadowModel.
R=einbinder, dgozman
Change-Id: Ic29a13073dc8236d5ef9a2fe35adf6bcd927290b
Reviewed-on: https://chromium-review.googlesource.com/1026882
Commit-Queue: Andrey Lushnikov <lushnikov@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#553468}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: b85cc4539f06a76af94addf2d193adaa4edc9b9d
chait pinnamaneni
