Ticket Back Full Screen PR Review - edmozley/freeitsm GitHub Wiki
Back from a full-screen ticket - PR review
PR #289 by Dr4inder ยท Follows #225 ยท Verdict: merge + fix
Back and Escape on a full-screen ticket now go straight back to the ticket list and leave full screen switched on, so the next ticket opens full screen again. User pages: Tickets ยง Pop-Out (Full-Screen) View and The ticket table. Internals: Ticket table - Developer Guide.
The problem it fixed
Back and Escape called toggleTicketPopout(), which is the layout switch: it saved tickets_popout = 0. So an analyst who worked full screen pressed Back once and the next ticket opened in the three-pane view. In Table mode it was worse: Back stopped at the ticket beside the table, and it took a second press to reach the list.
What the PR does
Piece (assets/js/inbox.js unless named) |
What it does |
|---|---|
backToTicketList() |
What Back and Escape now call. In Table mode it hands over to TicketTable.backToList(), otherwise leaveTicketPopout() |
leaveTicketPopout() |
Sets ticketPopoutPaused = true and takes the class off through syncPopoutToTicketState(false). It never writes the saved preference |
ticketPopoutPaused |
"Back was pressed - show the list until somebody opens a ticket on purpose". syncPopoutToTicketState() returns early (popout off) while it is set; toggleTicketPopout() and any deliberate open clear it |
| The shrink icon | Still toggleTicketPopout() - the one explicit way to switch full screen off |
selectEmail() "retained" path |
Reopening the ticket you just left, while paused or while the table is shown alone, reselects it and re-enters full screen without fetching or redrawing, so its DOM and any half-written reply survive |
selectEmail(id, {refresh: true}), loadTicketById(id, {refresh: true}) |
Every redraw that follows an action (move company, link / unlink problem, change or ticket, subject, send, wake, drop-assign, workspace switch, escalate) passes refresh. A refresh does not clear the pause or reopen the detail, so a reply arriving after Back cannot pop the ticket back over the list |
TicketTable.backToList() (assets/js/tickets-table.js) |
No longer awaits TicketWorkspace.canLeave() - it only hides the retained editor, so there is nothing to discard. It still waits while ticketPendingWrites > 0. canLeave() is asked when a different ticket is opened |
enterRow(), openRowFullScreen() |
Enter opens the ticket in the saved layout; Enter or double-click on the ticket already showing makes it full screen |
tests/ticket-back-navigation.php |
A browser test of the above |
The risk: live updates
The PR was branched before live updates (#2795, #267; LivePulse in inbox.js) went in, so both changed the same file in ways neither had seen.
- The text conflict was only the
?v=numbers intickets/index.php. Resolved asinbox.js?v=195,tickets-table.js?v=6. - The real question was semantic. Live updates redraw the open ticket when a colleague changes it:
liveReloadTicket()โdisplayEmail()โsyncPopoutToTicketState(true). Without the pause that would put an analyst who had pressed Back straight back into full screen whenever somebody else touched the ticket. The PR's early return insyncPopoutToTicketState()whileticketPopoutPausedis set covers it, so a colleague's change never pops the ticket back over the list. - Every other open. I went through every remaining
selectEmail/loadTicketByIdcall without{refresh: true}. All are deliberate opens - a click, Enter, the arrows, a URL, right-click Open - which should clear the pause.
What was checked
| Question | How | Result |
|---|---|---|
| Does the PR's own test pass on the merged code (with live updates)? | tests/ticket-back-navigation.php on the test install |
110/110 |
| The ticket table's live test, which the PR had not run | tests/ticket-table-live.php |
28/28 (its draft check now expects Back to keep the draft with no discard prompt) |
| Live updates | tests/live-pulse.php |
22/22 |
| Wider ticket screen | tests/ticket-workspace-live.php |
One failure, native internal note saves in the same ticket. It fails the same way on v4.12.0, so it was not caused by this PR - probably the test typing into a note box that is now a rich-text editor |
What was changed
- #2812 - the in-app help
tickets.help ... fullscreen_dblandfullscreen_backsaid Enter and Escape step in and out "one level at a time". Rewritten for the new behaviour in all six languages: Enter opens in the layout you last chose; Back and Escape return straight to the list; neither switches full screen off (the shrink icon does); going back to the ticket finds a started reply still there. Spanish, Indonesian and Polish had an older, shorter version of these two lines that the coverage audit could not see, because the keys existed - they now match English.
Merged with git merge --no-ff under the contributor's name (97efa5418), then the follow-up commit (713210a15).
Worth knowing
- ๐ด TRAP: any new code that redraws the open ticket after an action must pass
{refresh: true}toselectEmail()/loadTicketById(). Without it the call counts as a deliberate open: it clearsticketPopoutPausedand brings a ticket the analyst has just left back over the list. - ๐ด TRAP: anything that re-enters full screen must respect
ticketPopoutPaused. Go throughsyncPopoutToTicketState(), never add theticket-popoutclass directly. - Back is navigation, not a setting. Do not route Back or Escape through
toggleTicketPopout()again - that is what saved the preference off. - Back keeps the ticket's DOM. Reopening the same ticket takes the "retained" path and must not fetch and redraw, or a half-written reply is lost.
canLeave()belongs to opening a different ticket, not to Back.