Nourhan Hasan (Gerrit)

unread,

Nov 10, 2025, 7:43:15 AMNov 10

to Mathias Bynens, Philip Pfaffe, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Mathias Bynens, Nourhan Hasan and Philip Pfaffe

Nourhan Hasan voted and added 1 comment

Votes added by Nourhan Hasan

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Nourhan Hasan . resolved

Could you please review the changes I've made? Thank you in advance.

Open in Gerrit

Related details

Attention is currently required from:

Mathias Bynens
Nourhan Hasan
Philip Pfaffe

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Philip Pfaffe (Gerrit)

unread,

Nov 17, 2025, 4:01:24 AMNov 17

to Nourhan Hasan, Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin and Nourhan Hasan

Philip Pfaffe added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Philip Pfaffe . unresolved

I think this is generally fine but it papers over the underlying issue a bit. There's sort of two independent problems here, 1) autocomplete applying a style the user did not intend to, 2) that style not recovering after the user corrects the mistake. This CL here improves on 1), I think this is generally worth landing. @jacktf...@chromium.org WDYT?

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin
Nourhan Hasan

Submit Requirements:

Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Jack Franklin (Gerrit)

unread,

Nov 17, 2025, 4:45:45 AMNov 17

to Nourhan Hasan, Philip Pfaffe, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Nourhan Hasan

Jack Franklin voted and added 1 comment

Votes added by Jack Franklin

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Philip Pfaffe . unresolved

I think this is generally fine but it papers over the underlying issue a bit. There's sort of two independent problems here, 1) autocomplete applying a style the user did not intend to, 2) that style not recovering after the user corrects the mistake. This CL here improves on 1), I think this is generally worth landing. @jacktf...@chromium.org WDYT?

Jack Franklin

Yeah, I agree. I'd welcome a test for this case, but don't know how easy it is to add. I will defer to Philip on that but otherwise lgtm

Open in Gerrit

Related details

Attention is currently required from:

Nourhan Hasan

Submit Requirements:

Code-Owners

Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Philip Pfaffe (Gerrit)

unread,

Nov 17, 2025, 4:48:17 AMNov 17

to Nourhan Hasan, Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Nourhan Hasan

Philip Pfaffe added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Philip Pfaffe . unresolved

I think this is generally fine but it papers over the underlying issue a bit. There's sort of two independent problems here, 1) autocomplete applying a style the user did not intend to, 2) that style not recovering after the user corrects the mistake. This CL here improves on 1), I think this is generally worth landing. @jacktf...@chromium.org WDYT?

Jack Franklin

Yeah, I agree. I'd welcome a test for this case, but don't know how easy it is to add. I will defer to Philip on that but otherwise lgtm

Philip Pfaffe

SGTM, let's got forward with this then! And yes please add a test. panels/elements/StylesSidebarPane.test.ts has some tests for autocomplete that you could draw some inspiration from!

Open in Gerrit

Related details

Attention is currently required from:

Nourhan Hasan

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nourhan Hasan (Gerrit)

unread,

Nov 23, 2025, 5:02:39 AMNov 23

to Jack Franklin, Philip Pfaffe, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin and Philip Pfaffe

Nourhan Hasan added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Philip Pfaffe . unresolved

I think this is generally fine but it papers over the underlying issue a bit. There's sort of two independent problems here, 1) autocomplete applying a style the user did not intend to, 2) that style not recovering after the user corrects the mistake. This CL here improves on 1), I think this is generally worth landing. @jacktf...@chromium.org WDYT?

Jack Franklin

Yeah, I agree. I'd welcome a test for this case, but don't know how easy it is to add. I will defer to Philip on that but otherwise lgtm

Philip Pfaffe

SGTM, let's got forward with this then! And yes please add a test. panels/elements/StylesSidebarPane.test.ts has some tests for autocomplete that you could draw some inspiration from!

Nourhan Hasan

Thank you both. Is there anything I need to do to make this CL merge?

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin
Philip Pfaffe

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Philip Pfaffe (Gerrit)

unread,

Nov 24, 2025, 2:18:30 AMNov 24

to Nourhan Hasan, Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin and Nourhan Hasan

Philip Pfaffe added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Philip Pfaffe . unresolved

I think this is generally fine but it papers over the underlying issue a bit. There's sort of two independent problems here, 1) autocomplete applying a style the user did not intend to, 2) that style not recovering after the user corrects the mistake. This CL here improves on 1), I think this is generally worth landing. @jacktf...@chromium.org WDYT?

Jack Franklin

Yeah, I agree. I'd welcome a test for this case, but don't know how easy it is to add. I will defer to Philip on that but otherwise lgtm

Philip Pfaffe

SGTM, let's got forward with this then! And yes please add a test. panels/elements/StylesSidebarPane.test.ts has some tests for autocomplete that you could draw some inspiration from!

Nourhan Hasan

Thank you both. Is there anything I need to do to make this CL merge?

Philip Pfaffe

Do you think you could come up with a test?

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin
Nourhan Hasan

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Philip Pfaffe (Gerrit)

unread,

Nov 24, 2025, 2:23:27 AMNov 24

to Nourhan Hasan, Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin and Nourhan Hasan

Philip Pfaffe added 2 comments

Commit Message

Line 12, Patchset 1 (Latest):Fixed: Issue where typing height"100% would generate malformed CSS

Philip Pfaffe . unresolved

Nit: Automation detects a "Fixed:" prefix to reference bugs fixed by this CL.

Line 15, Patchset 1 (Latest):Bug: 338536264

Philip Pfaffe . unresolved

Nit: This belongs in the footer, i.e., the last block of the commit message next to Change-Id

satisfied_requirement

unsatisfied_requirement

open

diffy

Nourhan Hasan (Gerrit)

unread,

Nov 24, 2025, 9:36:23 AMNov 24

to Jack Franklin, Philip Pfaffe, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin and Philip Pfaffe

Nourhan Hasan added 1 comment

Patchset-level comments

File-level comment, Patchset 1:

Philip Pfaffe . unresolved

I think this is generally fine but it papers over the underlying issue a bit. There's sort of two independent problems here, 1) autocomplete applying a style the user did not intend to, 2) that style not recovering after the user corrects the mistake. This CL here improves on 1), I think this is generally worth landing. @jacktf...@chromium.org WDYT?

Jack Franklin

Yeah, I agree. I'd welcome a test for this case, but don't know how easy it is to add. I will defer to Philip on that but otherwise lgtm

Philip Pfaffe

SGTM, let's got forward with this then! And yes please add a test. panels/elements/StylesSidebarPane.test.ts has some tests for autocomplete that you could draw some inspiration from!

Nourhan Hasan

Thank you both. Is there anything I need to do to make this CL merge?

Philip Pfaffe

Do you think you could come up with a test?

Nourhan Hasan

I've done that. Could you please take a look and give me feedback? Thanks in advance.

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin
Philip Pfaffe

Submit Requirements:

Code-Owners

Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Philip Pfaffe (Gerrit)

unread,

Nov 25, 2025, 4:35:55 AMNov 25

to Nourhan Hasan, Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin and Nourhan Hasan

Philip Pfaffe voted and added 3 comments

Votes added by Philip Pfaffe

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 1:

Philip Pfaffe . resolved

I think this is generally fine but it papers over the underlying issue a bit. There's sort of two independent problems here, 1) autocomplete applying a style the user did not intend to, 2) that style not recovering after the user corrects the mistake. This CL here improves on 1), I think this is generally worth landing. @jacktf...@chromium.org WDYT?

Jack Franklin

Yeah, I agree. I'd welcome a test for this case, but don't know how easy it is to add. I will defer to Philip on that but otherwise lgtm

Philip Pfaffe

SGTM, let's got forward with this then! And yes please add a test. panels/elements/StylesSidebarPane.test.ts has some tests for autocomplete that you could draw some inspiration from!

Nourhan Hasan

Thank you both. Is there anything I need to do to make this CL merge?

Philip Pfaffe

Do you think you could come up with a test?

Nourhan Hasan

I've done that. Could you please take a look and give me feedback? Thanks in advance.

Philip Pfaffe

Looking good, thanks!

Commit Message

Line 12, Patchset 1:Fixed: Issue where typing height"100% would generate malformed CSS

Philip Pfaffe . resolved

Nit: Automation detects a "Fixed:" prefix to reference bugs fixed by this CL.

Philip Pfaffe

Done

Line 15, Patchset 1:Bug: 338536264

Philip Pfaffe . unresolved

Nit: This belongs in the footer, i.e., the last block of the commit message next to Change-Id

Philip Pfaffe

I'm not sure if the blank line works or not. Maybe remove it to be safe?

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin
Nourhan Hasan

Submit Requirements:

Code-Owners

Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nourhan Hasan (Gerrit)

unread,

Nov 25, 2025, 8:18:30 AMNov 25

to Philip Pfaffe, Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin

Nourhan Hasan added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Nourhan Hasan . resolved

I don't have access to CQ so could you land this CL for me please?

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nourhan Hasan (Gerrit)

unread,

Dec 12, 2025, 6:12:54 AM (9 days ago) Dec 12

to Philip Pfaffe, Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin

Nourhan Hasan added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Nourhan Hasan . resolved

@jacktf...@chromium.org Could you please take a look at this?

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jack Franklin (Gerrit)

unread,

Dec 12, 2025, 10:21:12 AM (9 days ago) Dec 12

to Nourhan Hasan, Philip Pfaffe, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Nourhan Hasan

Jack Franklin added 2 comments

Commit Message

Line 15, Patchset 1:Bug: 338536264

Philip Pfaffe . resolved

Nit: This belongs in the footer, i.e., the last block of the commit message next to Change-Id

Philip Pfaffe

I'm not sure if the blank line works or not. Maybe remove it to be safe?

Jack Franklin

Done

File front_end/panels/elements/StylesSidebarPane.test.ts

Line 725, Patchset 4 (Latest): if (spyObj?.updateSuggestions.called) {

Jack Franklin . unresolved

can we make this stronger? Do we expect this to be called? Right now if this isn't called, the test just passes. Is that correct?

Open in Gerrit

Related details

Attention is currently required from:

Nourhan Hasan

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nourhan Hasan (Gerrit)

unread,

Dec 13, 2025, 4:43:40 AM (9 days ago) Dec 13

to Philip Pfaffe, Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin

Nourhan Hasan added 1 comment

File front_end/panels/elements/StylesSidebarPane.test.ts

Line 725, Patchset 4 (Latest): if (spyObj?.updateSuggestions.called) {

Jack Franklin . unresolved

can we make this stronger? Do we expect this to be called? Right now if this isn't called, the test just passes. Is that correct?

Nourhan Hasan

Yes, you're correct, `updateSuggestions()` is't called for invalid input.

What do you think of this?
```
assert.isFalse(spyObj?.updateSuggestions.called,
          'Should not call updateSuggestions for invalid property names');
```

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Philip Pfaffe (Gerrit)

unread,

Dec 15, 2025, 3:17:50 AM (7 days ago) Dec 15

to Nourhan Hasan, Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin and Nourhan Hasan

Philip Pfaffe voted and added 1 comment

Votes added by Philip Pfaffe

Code-Review

+1

1 comment

File front_end/panels/elements/StylesSidebarPane.test.ts

Line 725, Patchset 4 (Latest): if (spyObj?.updateSuggestions.called) {

Jack Franklin . unresolved

can we make this stronger? Do we expect this to be called? Right now if this isn't called, the test just passes. Is that correct?

Nourhan Hasan

Yes, you're correct, `updateSuggestions()` is't called for invalid input.

What do you think of this?
```
assert.isFalse(spyObj?.updateSuggestions.called,
          'Should not call updateSuggestions for invalid property names');
```

Philip Pfaffe

I suggest to use `sinon.assert.notCalled` instead for a clean error message.

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin
Nourhan Hasan

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Nourhan Hasan (Gerrit)

unread,

4:27 AM (13 hours ago) 4:27 AM

to Philip Pfaffe, Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Attention needed from Jack Franklin and Philip Pfaffe

Nourhan Hasan added 1 comment

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Nourhan Hasan . resolved

Could you check please?
@pfa...@chromium.org @jacktf...@chromium.org

Open in Gerrit

Related details

Attention is currently required from:

Jack Franklin
Philip Pfaffe

Submit Requirements:

Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Fix CSS property autocomplete with invalid characters [devtools/devtools-frontend : main]

Nourhan Hasan (Gerrit)

Nourhan Hasan voted and added 1 comment

Votes added by Nourhan Hasan

1 comment

Related details

Philip Pfaffe (Gerrit)

Philip Pfaffe added 1 comment

Related details

Jack Franklin (Gerrit)

Jack Franklin voted and added 1 comment

Votes added by Jack Franklin

1 comment

Related details

Philip Pfaffe (Gerrit)

Philip Pfaffe added 1 comment

Related details

Nourhan Hasan (Gerrit)

Nourhan Hasan added 1 comment

Related details

Philip Pfaffe (Gerrit)

Philip Pfaffe added 1 comment

Related details

Philip Pfaffe (Gerrit)

Philip Pfaffe added 2 comments

Nourhan Hasan (Gerrit)

Nourhan Hasan added 1 comment

Related details

Philip Pfaffe (Gerrit)

Philip Pfaffe voted and added 3 comments

Votes added by Philip Pfaffe

3 comments

Related details

Nourhan Hasan (Gerrit)

Nourhan Hasan added 1 comment

Related details

Nourhan Hasan (Gerrit)

Nourhan Hasan added 1 comment

Related details

Jack Franklin (Gerrit)

Jack Franklin added 2 comments

Related details

Nourhan Hasan (Gerrit)

Nourhan Hasan added 1 comment

Related details

Philip Pfaffe (Gerrit)

Philip Pfaffe voted and added 1 comment

Votes added by Philip Pfaffe

1 comment

Related details

Nourhan Hasan (Gerrit)

Nourhan Hasan added 1 comment

Related details